Libguestfs - Oct 2010 - [PATCH] daemon: Fix /dev/mapper paths from mounts and mountpoints (RHBZ#646432).

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v
-------------- next part --------------
>From ebd89aa4c41634c2dfbf8475e4f35e4afb35b61f Mon Sep 17 00:00:00 2001
From: Richard W.M. Jones <rjones at redhat.com>
Date: Wed, 27 Oct 2010 14:51:17 +0100
Subject: [PATCH] daemon: Fix /dev/mapper paths from mounts and mountpoints
(RHBZ#646432).

Make the LV paths returned by these two commands canonical.
---
 daemon/daemon.h |    3 ++
 daemon/lvm.c    |   66 ++++++++++++++++++++++++++----------------------------
 daemon/mount.c  |   21 +++++++++++++++++
 3 files changed, 56 insertions(+), 34 deletions(-)

diff --git a/daemon/daemon.h b/daemon/daemon.h
index 03e0d37..e4e7159 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -124,6 +124,9 @@ extern struct optgroup optgroups[];
 /* Use this as a replacement for sync(2). */
 extern int sync_disks (void);
 
+/*-- in lvm.c --*/
+extern int lv_canonical (const char *device, char **ret);
+
 /*-- in proto.c --*/
 extern void main_loop (int sock) __attribute__((noreturn));
 
diff --git a/daemon/lvm.c b/daemon/lvm.c
index 2691daa..216c9c4 100644
--- a/daemon/lvm.c
+++ b/daemon/lvm.c
@@ -664,7 +664,8 @@ do_vgscan (void)
   return 0;
 }
 
-/* Test if a device is a logical volume (RHBZ#619793).
+/* Convert a non-canonical LV path like /dev/mapper/vg-lv or /dev/dm-0
+ * to a canonical one.
  *
  * This is harder than it should be.  A LV device like /dev/VG/LV is
  * really a symlink to a device-mapper device like /dev/dm-0.  However
@@ -675,9 +676,16 @@ do_vgscan (void)
  *
  * Note use of 'stat' instead of 'lstat' so that symlinks are
fully
  * resolved.
+ *
+ * Returns:
+ *   1  =  conversion was successful, path is an LV
+ *         '*ret' is set to the updated path if 'ret' is
non-NULL.
+ *   0  =  path is not an LV
+ *  -1  =  error, reply_with_* has been called
+ *
  */
 int
-do_is_lv (const char *device)
+lv_canonical (const char *device, char **ret)
 {
   struct stat stat1, stat2;
 
@@ -700,6 +708,14 @@ do_is_lv (const char *device)
       return -1;
     }
     if (stat1.st_rdev == stat2.st_rdev) { /* found it */
+      if (ret) {
+        *ret = strdup (lvs[i]);
+        if (*ret == NULL) {
+          reply_with_perror ("strdup");
+          free_strings (lvs);
+          return -1;
+        }
+      }
       free_strings (lvs);
       return 1;
     }
@@ -710,44 +726,26 @@ do_is_lv (const char *device)
   return 0;
 }
 
-/* Similar to is_lv above (RHBZ#638899). */
+/* Test if a device is a logical volume (RHBZ#619793). */
+int
+do_is_lv (const char *device)
+{
+  return lv_canonical (device, NULL);
+}
+
+/* Return canonical name of LV to caller (RHBZ#638899). */
 char *
 do_lvm_canonical_lv_name (const char *device)
 {
-  struct stat stat1, stat2;
-
-  int r = stat (device, &stat1);
-  if (r == -1) {
-    reply_with_perror ("stat: %s", device);
+  char *canonical;
+  int r = lv_canonical (device, &canonical);
+  if (r == -1)
     return NULL;
-  }
 
-  char **lvs = do_lvs ();
-  if (lvs == NULL)
+  if (r == 0) {
+    reply_with_error ("%s: not a logical volume", device);
     return NULL;
-
-  size_t i;
-  for (i = 0; lvs[i] != NULL; ++i) {
-    r = stat (lvs[i], &stat2);
-    if (r == -1) {
-      reply_with_perror ("stat: %s", lvs[i]);
-      free_strings (lvs);
-      return NULL;
-    }
-    if (stat1.st_rdev == stat2.st_rdev) { /* found it */
-      char *r = strdup (lvs[i]);
-      if (r == NULL) {
-        reply_with_perror ("strdup");
-        free_strings (lvs);
-      }
-      free_strings (lvs);
-      return r;
-    }
   }
 
-  free_strings (lvs);
-
-  /* not found */
-  reply_with_error ("%s: not a logical volume", device);
-  return NULL;
+  return canonical;             /* caller frees */
 }
diff --git a/daemon/mount.c b/daemon/mount.c
index 5485116..1c2ce65 100644
--- a/daemon/mount.c
+++ b/daemon/mount.c
@@ -149,6 +149,7 @@ mounts_or_mountpoints (int mp)
   char *p, *pend, *p2;
   int len;
   char matching[5 + sysroot_len];
+  size_t i;
 
   r = command (&out, &err, "mount", NULL);
   if (r == -1) {
@@ -204,6 +205,26 @@ mounts_or_mountpoints (int mp)
   if (add_string (&ret, &size, &alloc, NULL) == -1)
     return NULL;
 
+  /* Convert /dev/mapper LV paths into canonical paths (RHBZ#646432). */
+  for (i = 0; ret[i] != NULL; i += 2) {
+    if (STRPREFIX (ret[i], "/dev/mapper/") || STRPREFIX (ret[i],
"/dev/dm-")) {
+      char *canonical;
+      r = lv_canonical (ret[i], &canonical);
+      if (r == -1) {
+        free_strings (ret);
+        return NULL;
+      }
+      if (r == 1) {
+        free (ret[i]);
+        ret[i] = canonical;
+      }
+      /* Ignore the case where r == 0.  This might happen where
+       * eg. a LUKS /dev/mapper device is mounted, but that won't
+       * correspond to any LV.
+       */
+    }
+  }
+
   return ret;
 }
 
-- 
1.7.3.1

On Wed, Oct 27, 2010 at 02:53:07PM +0100, Richard W.M. Jones
wrote:
> +  for (i = 0; ret[i] != NULL; i += 2) {
This line in the patch is wrong.  It skips over pairs of strings,
which is good for 'mountpoints' but bad for 'mounts'.  The line
should
be:

+  for (i = 0; ret[i] != NULL; i += mp ? 2 : 1) {

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top