From c4c937d81c228c0f242ad4e8478064bafd4281c0 Mon Sep 17 00:00:00 2001 From: VinayBhaskar-V Date: Thu, 9 Jul 2026 22:10:06 +0530 Subject: [PATCH] librbd: propagate ENOENT for non-existent groups and images Calling `list_images()` on a non-existent group falsely succeeded and returned an empty iterator.This happened because `Group::image_list` ignored the negative error code returned by `group_image_list`, allowing execution to proceed blindly. Furthermore, the C API `rbd_group_image_list` intercepted `-ENOENT` and forced a success (0) return value. Similarly other public C APIs `rbd_snap_list` and `rbd_group_snap_list` also intercepts -ENOENT and forces a success (0) return value which is incorrect. Fix these behavior by: - Propagating errors properly inside `Group::image_list`. - Removing the incorrect `-ENOENT` masking in `rbd_group_image_list`, `rbd_snap_list`, rbd_group_snap_list` Now `list_images()` consistently raise an `ObjectNotFound` error when invoked on a non-existent group and public C APIs propagate ENOENT without masking it into 0. Also Extended the `TestGroups` test fixture with `self.dne_group` to validate the expected error paths across all relevant APIs. Fixes: https://tracker.ceph.com/issues/78108 Signed-off-by: VinayBhaskar-V --- PendingReleaseNotes | 4 ++++ src/librbd/api/Group.cc | 5 ++++- src/librbd/librbd.cc | 16 ---------------- src/test/librbd/test_Groups.cc | 12 ++++++++++++ src/test/pybind/test_rbd.py | 14 +++++++++++--- 5 files changed, 31 insertions(+), 20 deletions(-) diff --git a/PendingReleaseNotes b/PendingReleaseNotes index 9e873fab60c..9658c2f9964 100644 --- a/PendingReleaseNotes +++ b/PendingReleaseNotes @@ -269,6 +269,10 @@ rather rigid and also challenging to disseminate in some environments. The key can be embedded in the migration spec or just referenced from there while stored in the MON config-key store. +* RBD: `Group::list_images` python API will now correctly raise an ObjectNotFound + error when invoked on a non-existent group. The C APIs `rbd_group_image_list`, + `rbd_snap_list`, `rbd_group_snap_list` and C++ API `group_image_list` will now + return ENOENT instead of masking the error and returning 0. * BlueStore: The experimental PMEM device backend has been removed, together with the ``pmem`` value of the ``bdev_type`` option and the DML/DSA offload path. The hardware it targeted, Intel Optane DC persistent memory, was diff --git a/src/librbd/api/Group.cc b/src/librbd/api/Group.cc index 36c963941f1..c45471be6d4 100644 --- a/src/librbd/api/Group.cc +++ b/src/librbd/api/Group.cc @@ -822,7 +822,10 @@ int Group::image_list(librados::IoCtx& group_ioctx, std::vector image_ids; - group_image_list(group_ioctx, group_name, &image_ids); + int r = group_image_list(group_ioctx, group_name, &image_ids); + if (r < 0) { + return r; + } for (auto image_id : image_ids) { IoCtx ioctx; diff --git a/src/librbd/librbd.cc b/src/librbd/librbd.cc index fac2c5a2949..923261dd98d 100644 --- a/src/librbd/librbd.cc +++ b/src/librbd/librbd.cc @@ -5700,10 +5700,6 @@ extern "C" int rbd_snap_list(rbd_image_t image, rbd_snap_info_t *snaps, memset(snaps, 0, sizeof(*snaps) * *max_snaps); int r = librbd::api::Snapshot<>::list(ictx, cpp_snaps); - if (r == -ENOENT) { - tracepoint(librbd, snap_list_exit, 0, *max_snaps); - return 0; - } if (r < 0) { tracepoint(librbd, snap_list_exit, r, *max_snaps); return r; @@ -7266,12 +7262,6 @@ extern "C" int rbd_group_image_list(rados_ioctx_t group_p, int r = librbd::api::Group<>::image_list(group_ioctx, group_name, &cpp_images); - if (r == -ENOENT) { - tracepoint(librbd, group_image_list_exit, 0); - *image_size = 0; - return 0; - } - if (r < 0) { tracepoint(librbd, group_image_list_exit, r); return r; @@ -7447,12 +7437,6 @@ extern "C" int rbd_group_snap_list(rados_ioctx_t group_p, int r = librbd::api::Group<>::snap_list(group_ioctx, group_name, true, false, &cpp_snaps); - if (r == -ENOENT) { - *snaps_size = 0; - tracepoint(librbd, group_snap_list_exit, 0); - return 0; - } - if (r < 0) { tracepoint(librbd, group_snap_list_exit, r); return r; diff --git a/src/test/librbd/test_Groups.cc b/src/test/librbd/test_Groups.cc index 702128de181..122cda57bc5 100644 --- a/src/test/librbd/test_Groups.cc +++ b/src/test/librbd/test_Groups.cc @@ -164,6 +164,10 @@ TEST_F(TestGroup, add_image) ASSERT_EQ(1U, num_images); rbd_group_image_info_t images[1]; + ASSERT_EQ(-ENOENT, rbd_group_image_list(ioctx, "group_does_not_exist", + images, + sizeof(rbd_group_image_info_t), + &num_images)); ASSERT_EQ(1, rbd_group_image_list(ioctx, group_name, images, sizeof(rbd_group_image_info_t), &num_images)); @@ -233,6 +237,9 @@ TEST_F(TestGroup, add_imagePP) RBD_OPERATION_FEATURE_GROUP); std::vector images; + ASSERT_EQ(-ENOENT, rbd.group_image_list(ioctx, "group_does_not_exist", + &images, + sizeof(librbd::group_image_info_t))); ASSERT_EQ(0, rbd.group_image_list(ioctx, group_name, &images, sizeof(librbd::group_image_info_t))); ASSERT_EQ(1U, images.size()); @@ -342,6 +349,9 @@ TEST_F(TestGroup, add_snapshot) ASSERT_EQ(1U, num_snaps); rbd_group_snap_info_t snaps[1]; + ASSERT_EQ(-ENOENT, rbd_group_snap_list(ioctx, "group_does_not_exist", snaps, + sizeof(rbd_group_snap_info_t), + &num_snaps)); ASSERT_EQ(1, rbd_group_snap_list(ioctx, group_name, snaps, sizeof(rbd_group_snap_info_t), &num_snaps)); @@ -437,6 +447,8 @@ TEST_F(TestGroup, add_snapshotPP) ASSERT_EQ(0, rbd.group_snap_create(ioctx, group_name, snap_name)); std::vector snaps; + ASSERT_EQ(-ENOENT, rbd.group_snap_list(ioctx, "group_does_not_exist", &snaps, + sizeof(librbd::group_snap_info_t))); ASSERT_EQ(0, rbd.group_snap_list(ioctx, group_name, &snaps, sizeof(librbd::group_snap_info_t))); ASSERT_EQ(1U, snaps.size()); diff --git a/src/test/pybind/test_rbd.py b/src/test/pybind/test_rbd.py index 3bcadcb8481..8c90f0810c0 100644 --- a/src/test/pybind/test_rbd.py +++ b/src/test/pybind/test_rbd.py @@ -3087,12 +3087,11 @@ class TestGroups(object): RBD().remove(ioctx, name) def test_group_image_add(self): + assert_raises(ObjectNotFound, self.dne_group.add_image, ioctx, image_name) self.group.add_image(ioctx, image_name) - def test_group_image_list_empty(self): - eq([], list(self.group.list_images())) - def test_group_image_list(self): + assert_raises(ObjectNotFound, self.dne_group.list_images) eq([], list(self.group.list_images())) self.group.add_image(ioctx, image_name) eq([image_name], [img['name'] for img in self.group.list_images()]) @@ -3114,6 +3113,7 @@ class TestGroups(object): eq([image_name], [img['name'] for img in self.group.list_images()]) def test_group_get_id(self): + assert_raises(ObjectNotFound, self.dne_group.id) id = self.group.id() assert isinstance(id, str) assert len(id) > 0 @@ -3142,6 +3142,7 @@ class TestGroups(object): eq(group_name, group['name']) eq([image_name], [img['name'] for img in self.group.list_images()]) + assert_raises(ObjectNotFound, self.dne_group.remove_image, ioctx, image_name) self.group.remove_image(ioctx, image_name) eq([], list(self.group.list_images())) with Image(ioctx, image_name) as image: @@ -3157,6 +3158,7 @@ class TestGroups(object): assert_raises(ObjectNotFound, self.group.get_snap_info, "") self.group.create_snap(snap_name) + assert_raises(ObjectNotFound, self.dne_group.get_snap_info, snap_name) snap_info_dict = self.group.get_snap_info(snap_name) image_names = [] assert sorted(snap_info_dict.keys()) == self.gp_snap_keys @@ -3194,6 +3196,7 @@ class TestGroups(object): global snap_name assert_raises(ObjectNotFound, self.dne_group.list_snaps) eq([], list(self.group.list_snaps())) + assert_raises(ObjectNotFound, self.dne_group.create_snap, snap_name) self.group.create_snap(snap_name) eq([snap_name], [snap['name'] for snap in self.group.list_snaps()]) @@ -3203,6 +3206,7 @@ class TestGroups(object): eq(group_name, info['group_name']) eq(snap_name, info['group_snap_name']) + assert_raises(ObjectNotFound, self.dne_group.remove_snap, snap_name) self.group.remove_snap(snap_name) eq([], list(self.group.list_snaps())) @@ -3270,6 +3274,7 @@ class TestGroups(object): eq([], list(self.group.list_snaps())) self.group.create_snap(snap_name) eq([snap_name], [snap['name'] for snap in self.group.list_snaps()]) + assert_raises(ObjectNotFound, self.dne_group.rename_snap, snap_name, new_snap_name) self.group.rename_snap(snap_name, new_snap_name) eq([new_snap_name], [snap['name'] for snap in self.group.list_snaps()]) self.group.remove_snap(new_snap_name) @@ -3465,6 +3470,9 @@ class TestGroups(object): image_snaps = list(image.list_snaps()) assert [s['namespace'] for s in image_snaps] == [RBD_SNAP_NAMESPACE_TYPE_GROUP] + # no group exists + assert_raises(ObjectNotFound, self.dne_group.rollback_to_snap, snap_name1) + # group = [] assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name1) assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name2)