krbd: rewrite "already mapped" code

The "already mapped" code, introduced in commit d6a66fc8f49b ("rbd:
before rbd map, warn if the image is already mapped") is broken:
because of a use-after-free on attribute strings, the warning isn't
even printed half the time.

Rewrite making use of udev enumeration filters and fix the interface
while at it.

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

diff --git a/src/include/krbd.h b/src/include/krbd.h
index 4f6da4fbfafe1e7e64da44d7ec69a3e65840e4cd..eb753f5b684d7728a2e13f6d2a4b6903f8ae95e7 100644 (file)
--- a/src/include/krbd.h
+++ b/src/include/krbd.h
@@ -26,6 +26,8 @@ void krbd_destroy(struct krbd_ctx *ctx);
 
 int krbd_map(struct krbd_ctx *ctx, const char *pool, const char *image,
              const char *snap, const char *options, char **pdevnode);
+int krbd_is_mapped(struct krbd_ctx *ctx, const char *pool, const char *image,
+                   const char *snap, char **pdevnode);
 
 int krbd_unmap(struct krbd_ctx *ctx, const char *devnode,
                const char *options);
@@ -45,10 +47,6 @@ namespace ceph {
 
 int krbd_showmapped(struct krbd_ctx *ctx, ceph::Formatter *f);
 
-int krbd_is_image_mapped(struct krbd_ctx *ctx, const char *poolname, 
-                         const char *imgname, const char *snapname,
-                         std::ostringstream &mapped_info, bool &is_mapped);
-
 #endif /* __cplusplus */
 
 #endif /* CEPH_KRBD_H */

diff --git a/src/krbd.cc b/src/krbd.cc
index 50292c18ff2d4568e55bee4747f315b001bb4d15..523c3db1f5f6dfe3b09fecc0c22580afd569bea8 100644 (file)
--- a/src/krbd.cc
+++ b/src/krbd.cc
@@ -713,6 +713,46 @@ int dump_images(struct krbd_ctx *ctx, Formatter *f)
   return r;
 }
 
+static int is_mapped_image(struct udev *udev, const char *pool,
+                           const char *image, const char *snap, string *pname)
+{
+  struct udev_enumerate *enm;
+  struct udev_list_entry *l;
+  int r;
+
+  if (strcmp(snap, "") == 0)
+    snap = "-";
+
+  enm = udev_enumerate_new(udev);
+  if (!enm)
+    return -ENOMEM;
+
+  r = enumerate_devices(enm, pool, image, snap);
+  if (r < 0)
+    goto out_enm;
+
+  l = udev_enumerate_get_list_entry(enm);
+  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;
+    }
+
+    r = 1;
+    *pname = get_kernel_rbd_name(udev_device_get_sysname(dev));
+    udev_device_unref(dev);
+  } else {
+    r = 0;  /* not mapped */
+  }
+
+out_enm:
+  udev_enumerate_unref(enm);
+  return r;
+}
+
 extern "C" int krbd_create_from_context(rados_config_t cct,
                                         struct krbd_ctx **pctx)
 {
@@ -777,58 +817,22 @@ int krbd_showmapped(struct krbd_ctx *ctx, Formatter *f)
   return dump_images(ctx, f);
 }
 
-int krbd_is_image_mapped(struct krbd_ctx *ctx, const char *poolname, 
-                         const char *imgname, const char *snapname,
-                         std::ostringstream &mapped_info, bool &is_mapped) {
-  struct udev_enumerate *enm;
-  struct udev_list_entry *l;
-  struct udev *udev = ctx->udev; 
-  const char *mapped_id, *mapped_pool, *mapped_image, *mapped_snap;
+extern "C" int krbd_is_mapped(struct krbd_ctx *ctx, const char *pool,
+                              const char *image, const char *snap,
+                              char **pdevnode)
+{
+  string name;
+  char *devnode;
   int r;
-  is_mapped = false;
-
-  enm = udev_enumerate_new(udev);
-  if(!enm)
-    return -ENOMEM;
-
-  r = udev_enumerate_add_match_subsystem(enm, "rbd");
-  if (r < 0)
-    goto out_enm;
-
-  r = udev_enumerate_scan_devices(enm);
-  if (r < 0)
-    goto out_enm;
 
-  udev_list_entry_foreach(l, udev_enumerate_get_list_entry(enm)) {
-    struct udev_device *dev;
+  r = is_mapped_image(ctx->udev, pool, image, snap, &name);
+  if (r <= 0)  /* error or not mapped */
+    return r;
 
-    dev = udev_device_new_from_syspath(udev, udev_list_entry_get_name(l));
-    if (dev) {
-      mapped_id = udev_device_get_sysname(dev);
-      mapped_pool = udev_device_get_sysattr_value(dev, "pool");
-      mapped_image = udev_device_get_sysattr_value(dev, "name");
-      mapped_snap = udev_device_get_sysattr_value(dev, "current_snap");
-      string kname = get_kernel_rbd_name(mapped_id);
+  devnode = strdup(name.c_str());
+  if (!devnode)
+    return -ENOMEM;
 
-      udev_device_unref(dev);
-      if (mapped_pool && poolname && strcmp(mapped_pool, poolname) == 0 &&
-          mapped_image && imgname && strcmp(mapped_image, imgname) == 0) {
-        if (!snapname || snapname[0] == '\0') {
-          mapped_info << "image " << *poolname << "/" << *imgname
-                      << " already mapped as " << kname;
-          is_mapped = true;
-          goto out_enm;
-        } else if (snapname && mapped_snap &&
-                  strcmp(snapname, mapped_snap) == 0) {
-          mapped_info << "image " << *poolname << "/" << *imgname << "@"
-                     << *snapname << " already mapped as " << kname;
-          is_mapped = true;
-          goto out_enm;
-        }
-      }
-    }
-  }
-out_enm:
-  udev_enumerate_unref(enm);
+  *pdevnode = devnode;
   return r;
 }

diff --git a/src/tools/rbd/action/Kernel.cc b/src/tools/rbd/action/Kernel.cc
index 7c8480703780d9d0c2f4efc0f3db475e16616b0e..a0ffca60f416096fd592a1e2c13493a2d026c51b 100644 (file)
--- a/src/tools/rbd/action/Kernel.cc
+++ b/src/tools/rbd/action/Kernel.cc
@@ -289,10 +289,9 @@ static int do_kernel_map(const char *poolname, const char *imgname,
 {
 #if defined(WITH_KRBD)
   struct krbd_ctx *krbd;
-  std::ostringstream oss, mapped_info;
+  std::ostringstream oss;
   char *devnode;
   int r;
-  bool img_mapped;
 
   r = krbd_create_from_context(g_ceph_context, &krbd);
   if (r < 0)
@@ -313,13 +312,14 @@ static int do_kernel_map(const char *poolname, const char *imgname,
     }
   }
 
-  r = krbd_is_image_mapped(krbd, poolname, imgname, snapname,
-                          mapped_info, img_mapped);
+  r = krbd_is_mapped(krbd, poolname, imgname, snapname, &devnode);
   if (r < 0) {
     std::cerr << "rbd: warning: can't get image map infomation: "
              << cpp_strerror(r) << std::endl;
-  } else if (img_mapped) {
-    std::cerr << "rbd: warning: " << mapped_info.str() << std::endl;
+  } else if (r > 0) {
+    std::cerr << "rbd: warning: image already mapped as " << devnode
+              << std::endl;
+    free(devnode);
   }
 
   r = krbd_map(krbd, poolname, imgname, snapname, oss.str().c_str(), &devnode);