Can you please explain why is it expected to operate on non-exsiting objects in the commit messsage?

added.

since the test case expect false value return, not error code.

What do you mean by "false value"? Error codes are used internally - the raw error code value (int) is returned as an answer to the client eventually.

Looking at the cmp_set_vals_noexist_str test:

  EXPECT_EQ(do_cmp_set_vals(oid, Mode::String, Op::EQ, {{"eq", value}}), 0);

0 is expected while Crimson returns -2.

I'm not sure I understand why Classic is returning 0 here since the object store actually returns -2 as well in omap_get_values:

  if (!o || !o->exists) {
    r = -ENOENT;
    goto out;
  }

I think that either:

1. Classic should also return -2 and the test itself should be fixed.
2. CEPH_OSD_OP_FLAG_FAILOK is set, which hides the error and returns 0 instead - (why?)

Going back to my previous comment:

Can you please explain why is it expected to operate on non-existing objects in the commit message?

We should not change the behavior because the tests expects so as the test itself might possibly be wrong.

Lastly, I think that my comment regarding adding ceph_test_cls_cmpomap into makecheck or the crimson suite was missed. What do you think?

0 is returned (when it shouldn't) for non-existing objects for classic:

	if (oi.is_omap()) {
	  osd->store->omap_get_values(ch, ghobject_t(soid), keys_to_get, &out);
	} // else return empty omap entries
        ...

The non-existing object is not an omap so no error is returned.

For Crimson:

  if (oi.is_omap()) {
    return store->omap_get_values(coll, ghobject_t{oi.soid}, keys_to_get);
  } else {
    return crimson::ct_error::enodata::make();
  }

and then:

    }).handle_error_interruptible(
      crimson::ct_error::enodata::handle([&osd_op] {
        ..
        osd_op.rval = 0;
        return ll_read_errorator::now();
      }),

We actually return enodata since this object is not an omap - but then we change the error code to zero (probably to align with classic).

I actually think we should return ENOENT here in both cases and change the test itself.
What do you think?

@Matan-B Can't change the behavior without auditing users first. OMAPGETVALS, OMAPGETHEADER, and OMAPGETKEYS all seem to behave the same way -- empty, but successful return.

Short term, I'd suggest matching classic's behavior since this is a public interface other components already rely on.

Longer term, I do agree that the behavior is inconsistent with most of the other read ops, but changing the behavior would mean auditing rgw and rbd to make sure it won't break anything.

Ah, so until the longer term solution is there we could do something like this:

  if (!os.exists || os.oi.is_whiteout()) {
    logger().debug("{}: object does not exist: {} returning empty result", __func__, os.oi.soid);
    // Although an error should be expected here since the object doesn't exist,
    // we want to match classic's behavior as other components already rely on it.
    // Return an empty, but successful return instead.
    //return crimson::ct_error::enoent::make();
    return ll_read_errorator::now();
  }

Instead of removing the check completely, we could keep it and explain what's going on to avoid future confusion here. Since the error "removal" is not trivial.
@athanatos, @liu-chunmei - What do you think?

As suggested earlier, let's also add this to our regular testing.

0 is returned (when it shouldn't) for non-existing objects for classic:

	if (oi.is_omap()) {
	  osd->store->omap_get_values(ch, ghobject_t(soid), keys_to_get, &out);
	} // else return empty omap entries
        ...

The non-existing object is not an omap so no error is returned.

For Crimson:

  if (oi.is_omap()) {
    return store->omap_get_values(coll, ghobject_t{oi.soid}, keys_to_get);
  } else {
    return crimson::ct_error::enodata::make();
  }

and then:

    }).handle_error_interruptible(
      crimson::ct_error::enodata::handle([&osd_op] {
        ..
        osd_op.rval = 0;
        return ll_read_errorator::now();
      }),

We actually return enodata since this object is not an omap - but then we change the error code to zero (probably to align with classic).

I actually think we should return ENOENT here in both cases and change the test itself.
What do you think?