From 08bf0b6288036f6270cd31bad324ebf69b817359 Mon Sep 17 00:00:00 2001 From: Ilya Dryomov Date: Wed, 9 Oct 2019 12:40:11 +0200 Subject: [PATCH] krbd: do away with explicit memory management Wrap udev_monitor, udev_enumerate and udev_device with std::unique_ptr. Signed-off-by: Ilya Dryomov --- src/krbd.cc | 335 ++++++++++++++++++++++------------------------------ 1 file changed, 143 insertions(+), 192 deletions(-) diff --git a/src/krbd.cc b/src/krbd.cc index 43f08070cb4..e3c0fc436d8 100644 --- a/src/krbd.cc +++ b/src/krbd.cc @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -46,6 +47,19 @@ static const int UDEV_BUF_SIZE = 1 << 20; /* doubled to 2M (SO_RCVBUFFORCE) */ +#define DEFINE_UDEV_UPTR(what) \ +struct udev_##what##_deleter { \ + void operator()(udev_##what *p) { \ + udev_##what##_unref(p); \ + } \ +}; \ +using udev_##what##_uptr = \ + std::unique_ptr; + +DEFINE_UDEV_UPTR(monitor) /* udev_monitor_uptr */ +DEFINE_UDEV_UPTR(enumerate) /* udev_enumerate_uptr */ +DEFINE_UDEV_UPTR(device) /* udev_device_uptr */ + struct krbd_ctx { CephContext *cct; struct udev *udev; @@ -97,6 +111,12 @@ std::optional spec_from_dev(udev_device *dev) { pool_name, nspace_name ?: "", image_name, snap_name); } +static udev_device_uptr dev_from_list_entry(udev *udev, udev_list_entry *l) +{ + return udev_device_uptr( + udev_device_new_from_syspath(udev, udev_list_entry_get_name(l))); +} + static string get_kernel_rbd_name(const char *id) { return string("/dev/rbd") + id; @@ -270,9 +290,7 @@ static std::pair wait_for_mapping(int sysfs_r_fd, udev_monitor *mon, if (fds[1].revents) { for (;;) { - struct udev_device *dev; - - dev = udev_monitor_receive_device(mon); + udev_device_uptr dev(udev_monitor_receive_device(mon)); if (!dev) { if (errno != EINTR && errno != EAGAIN) { udev_r = -errno; @@ -284,7 +302,7 @@ static std::pair wait_for_mapping(int sysfs_r_fd, udev_monitor *mon, } break; } - if (udev_device_handler(dev)) { + if (udev_device_handler(std::move(dev))) { udev_r = 0; if (sysfs_r != INT_MAX) { ceph_assert(!sysfs_r); @@ -308,42 +326,40 @@ public: * block device to show up. This is necessary because rbd devices * and block devices aren't linked together in our sysfs layout. */ - bool operator()(udev_device *dev) { - if (strcmp(udev_device_get_action(dev), "add")) { - goto next; + bool operator()(udev_device_uptr dev) { + if (strcmp(udev_device_get_action(dev.get()), "add")) { + return false; } - if (!strcmp(udev_device_get_subsystem(dev), "rbd")) { + if (!strcmp(udev_device_get_subsystem(dev.get()), "rbd")) { if (!m_bus_dev) { - auto spec = spec_from_dev(dev); + auto spec = spec_from_dev(dev.get()); if (spec && *spec == *m_spec) { - m_bus_dev = dev; + m_bus_dev = std::move(dev); goto check; } } - } else if (!strcmp(udev_device_get_subsystem(dev), "block")) { - m_block_devs.push_back(dev); + } else if (!strcmp(udev_device_get_subsystem(dev.get()), "block")) { + m_block_devs.push_back(std::move(dev)); goto check; } -next: - udev_device_unref(dev); return false; check: if (m_bus_dev && !m_block_devs.empty()) { - const char *major = udev_device_get_sysattr_value(m_bus_dev, "major"); - const char *minor = udev_device_get_sysattr_value(m_bus_dev, "minor"); + const char *major = udev_device_get_sysattr_value(m_bus_dev.get(), "major"); + const char *minor = udev_device_get_sysattr_value(m_bus_dev.get(), "minor"); ceph_assert(!minor ^ have_minor_attr()); - for (auto p : m_block_devs) { - const char *this_major = udev_device_get_property_value(p, "MAJOR"); - const char *this_minor = udev_device_get_property_value(p, "MINOR"); + for (const auto& p : m_block_devs) { + const char *this_major = udev_device_get_property_value(p.get(), "MAJOR"); + const char *this_minor = udev_device_get_property_value(p.get(), "MINOR"); if (strcmp(this_major, major) == 0 && (!minor || strcmp(this_minor, minor) == 0)) { - string name = get_kernel_rbd_name(udev_device_get_sysname(m_bus_dev)); + string name = get_kernel_rbd_name(udev_device_get_sysname(m_bus_dev.get())); - ceph_assert(strcmp(udev_device_get_devnode(p), name.c_str()) == 0); + ceph_assert(strcmp(udev_device_get_devnode(p.get()), name.c_str()) == 0); *m_pdevnode = name; return true; } @@ -352,19 +368,9 @@ check: return false; } - ~UdevMapHandler() { - if (m_bus_dev) { - udev_device_unref(m_bus_dev); - } - - for (auto p : m_block_devs) { - udev_device_unref(p); - } - } - private: - udev_device *m_bus_dev = nullptr; - std::vector m_block_devs; + udev_device_uptr m_bus_dev; + std::vector m_block_devs; const krbd_spec *m_spec; std::string *m_pdevnode; }; @@ -372,41 +378,39 @@ private: static int do_map(struct udev *udev, const krbd_spec& spec, const string& buf, string *pname) { - struct udev_monitor *mon; - std::thread mapper; bool mapped; int fds[2]; int r; - mon = udev_monitor_new_from_netlink(udev, "udev"); + udev_monitor_uptr mon(udev_monitor_new_from_netlink(udev, "udev")); if (!mon) return -ENOMEM; - r = udev_monitor_filter_add_match_subsystem_devtype(mon, "rbd", nullptr); + r = udev_monitor_filter_add_match_subsystem_devtype(mon.get(), "rbd", + nullptr); if (r < 0) - goto out_mon; + return r; - r = udev_monitor_filter_add_match_subsystem_devtype(mon, "block", "disk"); + r = udev_monitor_filter_add_match_subsystem_devtype(mon.get(), "block", + "disk"); if (r < 0) - goto out_mon; + return r; - r = udev_monitor_set_receive_buffer_size(mon, UDEV_BUF_SIZE); + r = udev_monitor_set_receive_buffer_size(mon.get(), UDEV_BUF_SIZE); if (r < 0) { std::cerr << "rbd: failed to set udev buffer size: " << cpp_strerror(r) << std::endl; /* not fatal */ } - r = udev_monitor_enable_receiving(mon); + r = udev_monitor_enable_receiving(mon.get()); if (r < 0) - goto out_mon; + return r; - if (pipe2(fds, O_NONBLOCK) < 0) { - r = -errno; - goto out_mon; - } + if (pipe2(fds, O_NONBLOCK) < 0) + return -errno; - mapper = make_named_thread("mapper", [&buf, sysfs_r_fd = fds[1]]() { + auto mapper = make_named_thread("mapper", [&buf, sysfs_r_fd = fds[1]]() { int sysfs_r = sysfs_write_rbd_add(buf); int r = safe_write(sysfs_r_fd, &sysfs_r, sizeof(sysfs_r)); if (r < 0) { @@ -414,7 +418,7 @@ static int do_map(struct udev *udev, const krbd_spec& spec, const string& buf, } }); - std::tie(r, mapped) = wait_for_mapping(fds[0], mon, + std::tie(r, mapped) = wait_for_mapping(fds[0], mon.get(), UdevMapHandler(&spec, pname)); if (r < 0) { if (!mapped) { @@ -428,9 +432,6 @@ static int do_map(struct udev *udev, const krbd_spec& spec, const string& buf, mapper.join(); close(fds[0]); close(fds[1]); - -out_mon: - udev_monitor_unref(mon); return r; } @@ -472,128 +473,115 @@ static int map_image(struct krbd_ctx *ctx, const krbd_spec& spec, static int devno_to_krbd_id(struct udev *udev, dev_t devno, string *pid) { - struct udev_enumerate *enm; + udev_enumerate_uptr enm; struct udev_list_entry *l; - struct udev_device *dev; int r; retry: - enm = udev_enumerate_new(udev); + enm.reset(udev_enumerate_new(udev)); if (!enm) return -ENOMEM; - r = udev_enumerate_add_match_subsystem(enm, "rbd"); + r = udev_enumerate_add_match_subsystem(enm.get(), "rbd"); if (r < 0) - goto out_enm; + return r; - r = udev_enumerate_add_match_sysattr(enm, "major", + r = udev_enumerate_add_match_sysattr(enm.get(), "major", stringify(major(devno)).c_str()); if (r < 0) - goto out_enm; + return r; if (have_minor_attr()) { - r = udev_enumerate_add_match_sysattr(enm, "minor", + r = udev_enumerate_add_match_sysattr(enm.get(), "minor", stringify(minor(devno)).c_str()); if (r < 0) - goto out_enm; + return r; } - r = udev_enumerate_scan_devices(enm); + r = udev_enumerate_scan_devices(enm.get()); if (r < 0) { if (r == -ENOENT || r == -ENODEV) { std::cerr << "rbd: udev enumerate failed, retrying" << std::endl; - udev_enumerate_unref(enm); goto retry; } - goto out_enm; + return r; } - l = udev_enumerate_get_list_entry(enm); - if (!l) { - r = -ENOENT; - goto out_enm; - } + l = udev_enumerate_get_list_entry(enm.get()); + if (!l) + return -ENOENT; /* make sure there is only one match */ ceph_assert(!udev_list_entry_get_next(l)); - dev = udev_device_new_from_syspath(udev, udev_list_entry_get_name(l)); - if (!dev) { - r = -ENOMEM; - goto out_enm; - } - - *pid = udev_device_get_sysname(dev); + auto dev = dev_from_list_entry(udev, l); + if (!dev) + return -ENOMEM; - udev_device_unref(dev); -out_enm: - udev_enumerate_unref(enm); - return r; + *pid = udev_device_get_sysname(dev.get()); + return 0; } static int __enumerate_devices(struct udev *udev, const krbd_spec& spec, - bool match_nspace, struct udev_enumerate **penm) + bool match_nspace, udev_enumerate_uptr *penm) { - struct udev_enumerate *enm; + udev_enumerate_uptr enm; int r; retry: - enm = udev_enumerate_new(udev); + enm.reset(udev_enumerate_new(udev)); if (!enm) return -ENOMEM; - r = udev_enumerate_add_match_subsystem(enm, "rbd"); + r = udev_enumerate_add_match_subsystem(enm.get(), "rbd"); if (r < 0) - goto out_enm; + return r; - r = udev_enumerate_add_match_sysattr(enm, "pool", spec.pool_name.c_str()); + r = udev_enumerate_add_match_sysattr(enm.get(), "pool", + spec.pool_name.c_str()); if (r < 0) - goto out_enm; + return r; if (match_nspace) { - r = udev_enumerate_add_match_sysattr(enm, "pool_ns", + r = udev_enumerate_add_match_sysattr(enm.get(), "pool_ns", spec.nspace_name.c_str()); } else { /* * Match _only_ devices that don't have pool_ns attribute. * If the kernel supports namespaces, the result will be empty. */ - r = udev_enumerate_add_nomatch_sysattr(enm, "pool_ns", nullptr); + r = udev_enumerate_add_nomatch_sysattr(enm.get(), "pool_ns", nullptr); } if (r < 0) - goto out_enm; + return r; - r = udev_enumerate_add_match_sysattr(enm, "name", spec.image_name.c_str()); + r = udev_enumerate_add_match_sysattr(enm.get(), "name", + spec.image_name.c_str()); if (r < 0) - goto out_enm; + return r; - r = udev_enumerate_add_match_sysattr(enm, "current_snap", + r = udev_enumerate_add_match_sysattr(enm.get(), "current_snap", spec.snap_name.c_str()); if (r < 0) - goto out_enm; + return r; - r = udev_enumerate_scan_devices(enm); + r = udev_enumerate_scan_devices(enm.get()); if (r < 0) { if (r == -ENOENT || r == -ENODEV) { std::cerr << "rbd: udev enumerate failed, retrying" << std::endl; - udev_enumerate_unref(enm); goto retry; } - goto out_enm; + return r; } - *penm = enm; + *penm = std::move(enm); return 0; - -out_enm: - udev_enumerate_unref(enm); - return r; } static int enumerate_devices(struct udev *udev, const krbd_spec& spec, - struct udev_enumerate **penm) + udev_enumerate_uptr *penm) { - struct udev_enumerate *enm; + udev_enumerate_uptr enm; int r; r = __enumerate_devices(udev, spec, true, &enm); @@ -605,23 +593,21 @@ static int enumerate_devices(struct udev *udev, const krbd_spec& spec, * handle older kernels. On a newer kernel the result will remain * the same (i.e. empty). */ - if (!udev_enumerate_get_list_entry(enm) && spec.nspace_name.empty()) { - udev_enumerate_unref(enm); + if (!udev_enumerate_get_list_entry(enm.get()) && spec.nspace_name.empty()) { r = __enumerate_devices(udev, spec, false, &enm); if (r < 0) return r; } - *penm = enm; + *penm = std::move(enm); return 0; } static int spec_to_devno_and_krbd_id(struct udev *udev, const krbd_spec& spec, dev_t *pdevno, string *pid) { - struct udev_enumerate *enm; + udev_enumerate_uptr enm; struct udev_list_entry *l; - struct udev_device *dev; unsigned int maj, min = 0; string err; int r; @@ -630,30 +616,26 @@ static int spec_to_devno_and_krbd_id(struct udev *udev, const krbd_spec& spec, if (r < 0) return r; - l = udev_enumerate_get_list_entry(enm); - if (!l) { - r = -ENOENT; - goto out_enm; - } + l = udev_enumerate_get_list_entry(enm.get()); + if (!l) + return -ENOENT; - dev = udev_device_new_from_syspath(udev, udev_list_entry_get_name(l)); - if (!dev) { - r = -ENOMEM; - goto out_enm; - } + auto dev = dev_from_list_entry(udev, l); + if (!dev) + return -ENOMEM; - maj = strict_strtoll(udev_device_get_sysattr_value(dev, "major"), 10, &err); + maj = strict_strtoll(udev_device_get_sysattr_value(dev.get(), "major"), 10, + &err); if (!err.empty()) { cerr << "rbd: couldn't parse major: " << err << std::endl; - r = -EINVAL; - goto out_dev; + return -EINVAL; } if (have_minor_attr()) { - min = strict_strtoll(udev_device_get_sysattr_value(dev, "minor"), 10, &err); + min = strict_strtoll(udev_device_get_sysattr_value(dev.get(), "minor"), 10, + &err); if (!err.empty()) { cerr << "rbd: couldn't parse minor: " << err << std::endl; - r = -EINVAL; - goto out_dev; + return -EINVAL; } } @@ -664,17 +646,12 @@ static int spec_to_devno_and_krbd_id(struct udev *udev, const krbd_spec& spec, */ if (udev_list_entry_get_next(l)) cerr << "rbd: " << spec << ": mapped more than once, unmapping " - << get_kernel_rbd_name(udev_device_get_sysname(dev)) + << get_kernel_rbd_name(udev_device_get_sysname(dev.get())) << " only" << std::endl; *pdevno = makedev(maj, min); - *pid = udev_device_get_sysname(dev); - -out_dev: - udev_device_unref(dev); -out_enm: - udev_enumerate_unref(enm); - return r; + *pid = udev_device_get_sysname(dev.get()); + return 0; } static string build_unmap_buf(const string& id, const char *options) @@ -691,15 +668,11 @@ class UdevUnmapHandler { public: UdevUnmapHandler(dev_t devno) : m_devno(devno) {} - bool operator()(udev_device *dev) { - bool match = false; - - if (!strcmp(udev_device_get_action(dev), "remove") && - udev_device_get_devnum(dev) == m_devno) { - match = true; + bool operator()(udev_device_uptr dev) { + if (strcmp(udev_device_get_action(dev.get()), "remove")) { + return false; } - udev_device_unref(dev); - return match; + return udev_device_get_devnum(dev.get()) == m_devno; } private: @@ -708,37 +681,34 @@ private: static int do_unmap(struct udev *udev, dev_t devno, const string& buf) { - struct udev_monitor *mon; - std::thread unmapper; bool unmapped; int fds[2]; int r; - mon = udev_monitor_new_from_netlink(udev, "udev"); + udev_monitor_uptr mon(udev_monitor_new_from_netlink(udev, "udev")); if (!mon) return -ENOMEM; - r = udev_monitor_filter_add_match_subsystem_devtype(mon, "block", "disk"); + r = udev_monitor_filter_add_match_subsystem_devtype(mon.get(), "block", + "disk"); if (r < 0) - goto out_mon; + return r; - r = udev_monitor_set_receive_buffer_size(mon, UDEV_BUF_SIZE); + r = udev_monitor_set_receive_buffer_size(mon.get(), UDEV_BUF_SIZE); if (r < 0) { std::cerr << "rbd: failed to set udev buffer size: " << cpp_strerror(r) << std::endl; /* not fatal */ } - r = udev_monitor_enable_receiving(mon); + r = udev_monitor_enable_receiving(mon.get()); if (r < 0) - goto out_mon; + return r; - if (pipe2(fds, O_NONBLOCK) < 0) { - r = -errno; - goto out_mon; - } + if (pipe2(fds, O_NONBLOCK) < 0) + return -errno; - unmapper = make_named_thread("unmapper", [&buf, sysfs_r_fd = fds[1]]() { + auto unmapper = make_named_thread("unmapper", [&buf, sysfs_r_fd = fds[1]]() { /* * On final device close(), kernel sends a block change event, in * response to which udev apparently runs blkid on the device. This @@ -770,7 +740,7 @@ static int do_unmap(struct udev *udev, dev_t devno, const string& buf) } }); - std::tie(r, unmapped) = wait_for_mapping(fds[0], mon, + std::tie(r, unmapped) = wait_for_mapping(fds[0], mon.get(), UdevUnmapHandler(devno)); if (r < 0) { if (!unmapped) { @@ -784,9 +754,6 @@ static int do_unmap(struct udev *udev, dev_t devno, const string& buf) unmapper.join(); close(fds[0]); close(fds[1]); - -out_mon: - udev_monitor_unref(mon); return r; } @@ -898,44 +865,37 @@ static bool dump_one_image(Formatter *f, TextTable *tbl, static int do_dump(struct udev *udev, Formatter *f, TextTable *tbl) { - struct udev_enumerate *enm; + udev_enumerate_uptr enm; struct udev_list_entry *l = NULL; bool have_output = false; int r; retry: - enm = udev_enumerate_new(udev); + enm.reset(udev_enumerate_new(udev)); if (!enm) return -ENOMEM; - r = udev_enumerate_add_match_subsystem(enm, "rbd"); + r = udev_enumerate_add_match_subsystem(enm.get(), "rbd"); if (r < 0) - goto out_enm; + return r; - r = udev_enumerate_scan_devices(enm); + r = udev_enumerate_scan_devices(enm.get()); if (r < 0) { if (r == -ENOENT || r == -ENODEV) { std::cerr << "rbd: udev enumerate failed, retrying" << std::endl; - udev_enumerate_unref(enm); goto retry; } - goto out_enm; + return r; } - udev_list_entry_foreach(l, udev_enumerate_get_list_entry(enm)) { - struct udev_device *dev; - - dev = udev_device_new_from_syspath(udev, udev_list_entry_get_name(l)); + udev_list_entry_foreach(l, udev_enumerate_get_list_entry(enm.get())) { + auto dev = dev_from_list_entry(udev, l); if (dev) { - have_output |= dump_one_image(f, tbl, dev); - udev_device_unref(dev); + have_output |= dump_one_image(f, tbl, dev.get()); } } - r = have_output; -out_enm: - udev_enumerate_unref(enm); - return r; + return have_output; } int dump_images(struct krbd_ctx *ctx, Formatter *f) @@ -970,7 +930,7 @@ int dump_images(struct krbd_ctx *ctx, Formatter *f) static int is_mapped_image(struct udev *udev, const krbd_spec& spec, string *pname) { - struct udev_enumerate *enm; + udev_enumerate_uptr enm; struct udev_list_entry *l; int r; @@ -978,26 +938,17 @@ static int is_mapped_image(struct udev *udev, const krbd_spec& spec, if (r < 0) return r; - l = udev_enumerate_get_list_entry(enm); + l = udev_enumerate_get_list_entry(enm.get()); if (l) { - struct udev_device *dev; - - dev = udev_device_new_from_syspath(udev, udev_list_entry_get_name(l)); - if (!dev) { - r = -ENOMEM; - goto out_enm; - } + auto dev = dev_from_list_entry(udev, l); + if (!dev) + return -ENOMEM; - r = 1; - *pname = get_kernel_rbd_name(udev_device_get_sysname(dev)); - udev_device_unref(dev); - } else { - r = 0; /* not mapped */ + *pname = get_kernel_rbd_name(udev_device_get_sysname(dev.get())); + return 1; } -out_enm: - udev_enumerate_unref(enm); - return r; + return 0; /* not mapped */ } extern "C" int krbd_create_from_context(rados_config_t cct, -- 2.39.5