Libguestfs - Jan 2016 - Re: [PATCH] lvm: support lvm2 older than 2.02.107

lvm2 2.02.107 adds the -S/--select option used in lvs to filter out only
public LVs (see RHBZ#1278878).  To make this work again with versions
of lvm2 older than that, only on old versions filter out thin layouts
and compose the resulting device strings ourselves.

The filtering done is much simplier than what "-S lv_role=public" will
do, but should be good enough for our need.
---
 daemon/lvm.c | 150 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 140 insertions(+), 10 deletions(-)

diff --git a/daemon/lvm.c b/daemon/lvm.c
index 979cf63..604e106 100644
--- a/daemon/lvm.c
+++ b/daemon/lvm.c
@@ -26,6 +26,7 @@
 #include <fcntl.h>
 #include <sys/stat.h>
 #include <dirent.h>
+#include <assert.h>
 
 #include "daemon.h"
 #include "c-ctype.h"
@@ -103,6 +104,85 @@ convert_lvm_output (char *out, const char *prefix)
   return ret.argv;
 }
 
+/* Filter a colon-separated output of
+ *   lvs -o lv_attr,vg_name,lv_name
+ * removing thin layouts, and building the device path as we expect it.
+ *
+ * This is used only when lvm has no -S.
+ */
+static char **
+filter_convert_old_lvs_output (char *out)
+{
+  char *p, *pend;
+  DECLARE_STRINGSBUF (ret);
+
+  p = out;
+  while (p) {
+    size_t len;
+    char *saveptr;
+    char *lv_attr, *vg_name, *lv_name;
+
+    pend = strchr (p, '\n');	/* Get the next line of output. */
+    if (pend) {
+      *pend = '\0';
+      pend++;
+    }
+
+    while (*p && c_isspace (*p))	/* Skip any leading whitespace. */
+      p++;
+
+    /* Sigh, skip trailing whitespace too.  "pvs", I'm looking at
you. */
+    len = strlen (p)-1;
+    while (*p && c_isspace (p[len]))
+      p[len--] = '\0';
+
+    if (!*p) {			/* Empty line?  Skip it. */
+    skip_line:
+      p = pend;
+      continue;
+    }
+
+    lv_attr = strtok_r (p, ":", &saveptr);
+    if (!lv_attr)
+      goto skip_line;
+
+    vg_name = strtok_r (NULL, ":", &saveptr);
+    if (!vg_name)
+      goto skip_line;
+
+    lv_name = strtok_r (NULL, ":", &saveptr);
+    if (!lv_name)
+      goto skip_line;
+
+    /* Ignore thin layouts (RHBZ#1278878). */
+    if (lv_attr[0] == 't')
+      goto skip_line;
+
+    /* Ignore "unknown device" message (RHBZ#1054761). */
+    if (STRNEQ (p, "unknown device")) {
+      char buf[256];
+
+      snprintf (buf, sizeof buf, "/dev/%s/%s", vg_name, lv_name);
+      if (add_string (&ret, buf) == -1) {
+        free (out);
+        return NULL;
+      }
+    }
+
+    p = pend;
+  }
+
+  free (out);
+
+  if (ret.size > 0)
+    sort_strings (ret.argv, ret.size);
+
+  if (end_stringsbuf (&ret) == -1)
+    return NULL;
+
+  return ret.argv;
+}
+
 char **
 do_pvs (void)
 {
@@ -139,26 +219,76 @@ do_vgs (void)
   return convert_lvm_output (out, NULL);
 }
 
+/* Check whether lvs has -S to filter its output.
+ * It is available only in lvm2 >= 2.02.107.
+ */
+static int
+test_lvs_has_S_opt (void)
+{
+  static int result = -1;
+  if (result != -1)
+    return result;
+
+  CLEANUP_FREE char *out = NULL;
+  CLEANUP_FREE char *err = NULL;
+
+  int r = command (&out, &err, str_lvm, "lvs",
"--help", NULL);
+  if (r == -1) {
+    reply_with_error ("lvm lvs --help: %s", err);
+    return -1;
+  }
+
+  if (strstr (out, "-S") == NULL)
+    result = 0;
+  else
+    result = 1;
+
+  return result;
+}
+
 char **
 do_lvs (void)
 {
   char *out;
   CLEANUP_FREE char *err = NULL;
   int r;
+  int has_S = test_lvs_has_S_opt ();
 
-  r = command (&out, &err,
-               str_lvm, "lvs",
-               "-o", "vg_name,lv_name",
-               "-S", "lv_role=public",
-               "--noheadings",
-               "--separator", "/", NULL);
-  if (r == -1) {
-    reply_with_error ("%s", err);
-    free (out);
+  if (has_S < 0)
     return NULL;
+
+  if (has_S > 0) {
+    r = command (&out, &err,
+                 str_lvm, "lvs",
+                 "-o", "vg_name,lv_name",
+                 "-S", "lv_role=public",
+                 "--noheadings",
+                 "--separator", "/", NULL);
+    if (r == -1) {
+      reply_with_error ("%s", err);
+      free (out);
+      return NULL;
+    }
+
+    return convert_lvm_output (out, "/dev/");
+  } else {
+    r = command (&out, &err,
+                 str_lvm, "lvs",
+                 "-o", "lv_attr,vg_name,lv_name",
+                 "--noheadings",
+                 "--separator", ":", NULL);
+    if (r == -1) {
+      reply_with_error ("%s", err);
+      free (out);
+      return NULL;
+    }
+
+    return filter_convert_old_lvs_output (out);
   }
 
-  return convert_lvm_output (out, "/dev/");
+  /*NOTREACHED*/
+  assert (false);
+  return NULL;
 }
 
 /* These were so complex to implement that I ended up auto-generating
-- 
2.5.0

On Wed, Jan 27, 2016 at 07:06:46PM +0100, Pino Toscano
wrote:
>  char **
>  do_lvs (void)
>  {
>    char *out;
>    CLEANUP_FREE char *err = NULL;
>    int r;
> +  int has_S = test_lvs_has_S_opt ();
> +  if (has_S < 0)
>      return NULL;
I think this doesn't do the right thing on the second failing call
to test_lvs_has_S_opt, because it won't return a protocol error?

> +  /*NOTREACHED*/
> +  assert (false);
> +  return NULL;
>  }
What's this bit for?  It is indeed NOTREACHED, but it seems as if
the 3 lines could just be removed.

Rest looks fine, so ACK with those fixed.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Thursday 28 January 2016 13:47:30 Richard W.M. Jones
wrote:
> On Wed, Jan 27, 2016 at 07:06:46PM +0100, Pino Toscano wrote:
> >  char **
> >  do_lvs (void)
> >  {
> >    char *out;
> >    CLEANUP_FREE char *err = NULL;
> >    int r;
> > +  int has_S = test_lvs_has_S_opt ();
> > +  if (has_S < 0)
> >      return NULL;
> 
> I think this doesn't do the right thing on the second failing call
> to test_lvs_has_S_opt, because it won't return a protocol error?
test_lvs_has_S_opt caches the result only if != -1, so if executing
"lvm lvs --help" fails then an error is replied and nothing is cached;
next time the command will be tried again.
> > +  /*NOTREACHED*/
> > +  assert (false);
> > +  return NULL;
> >  }
> 
> What's this bit for?  It is indeed NOTREACHED, but it seems as if
> the 3 lines could just be removed.
It was a simple extra safety belt, since IIRC past versions of GCC
could not recognize such case was indeed not reachable. I guess this
can be added in case it is misreported.

-- 
Pino Toscano

lvm2 2.02.107 adds the -S/--select option used in lvs to filter out only
public LVs (see RHBZ#1278878).  To make this work again with versions
of lvm2 older than that, only on old versions filter out thin layouts
and compose the resulting device strings ourselves.

The filtering done is much simplier than what "-S lv_role=public" will
do, but should be good enough for our need.
---
 daemon/lvm.c | 147 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 136 insertions(+), 11 deletions(-)

diff --git a/daemon/lvm.c b/daemon/lvm.c
index 979cf63..8bef4d5 100644
--- a/daemon/lvm.c
+++ b/daemon/lvm.c
@@ -103,6 +103,85 @@ convert_lvm_output (char *out, const char *prefix)
   return ret.argv;
 }
 
+/* Filter a colon-separated output of
+ *   lvs -o lv_attr,vg_name,lv_name
+ * removing thin layouts, and building the device path as we expect it.
+ *
+ * This is used only when lvm has no -S.
+ */
+static char **
+filter_convert_old_lvs_output (char *out)
+{
+  char *p, *pend;
+  DECLARE_STRINGSBUF (ret);
+
+  p = out;
+  while (p) {
+    size_t len;
+    char *saveptr;
+    char *lv_attr, *vg_name, *lv_name;
+
+    pend = strchr (p, '\n');	/* Get the next line of output. */
+    if (pend) {
+      *pend = '\0';
+      pend++;
+    }
+
+    while (*p && c_isspace (*p))	/* Skip any leading whitespace. */
+      p++;
+
+    /* Sigh, skip trailing whitespace too.  "pvs", I'm looking at
you. */
+    len = strlen (p)-1;
+    while (*p && c_isspace (p[len]))
+      p[len--] = '\0';
+
+    if (!*p) {			/* Empty line?  Skip it. */
+    skip_line:
+      p = pend;
+      continue;
+    }
+
+    lv_attr = strtok_r (p, ":", &saveptr);
+    if (!lv_attr)
+      goto skip_line;
+
+    vg_name = strtok_r (NULL, ":", &saveptr);
+    if (!vg_name)
+      goto skip_line;
+
+    lv_name = strtok_r (NULL, ":", &saveptr);
+    if (!lv_name)
+      goto skip_line;
+
+    /* Ignore thin layouts (RHBZ#1278878). */
+    if (lv_attr[0] == 't')
+      goto skip_line;
+
+    /* Ignore "unknown device" message (RHBZ#1054761). */
+    if (STRNEQ (p, "unknown device")) {
+      char buf[256];
+
+      snprintf (buf, sizeof buf, "/dev/%s/%s", vg_name, lv_name);
+      if (add_string (&ret, buf) == -1) {
+        free (out);
+        return NULL;
+      }
+    }
+
+    p = pend;
+  }
+
+  free (out);
+
+  if (ret.size > 0)
+    sort_strings (ret.argv, ret.size);
+
+  if (end_stringsbuf (&ret) == -1)
+    return NULL;
+
+  return ret.argv;
+}
+
 char **
 do_pvs (void)
 {
@@ -139,26 +218,72 @@ do_vgs (void)
   return convert_lvm_output (out, NULL);
 }
 
+/* Check whether lvs has -S to filter its output.
+ * It is available only in lvm2 >= 2.02.107.
+ */
+static int
+test_lvs_has_S_opt (void)
+{
+  static int result = -1;
+  if (result != -1)
+    return result;
+
+  CLEANUP_FREE char *out = NULL;
+  CLEANUP_FREE char *err = NULL;
+
+  int r = command (&out, &err, str_lvm, "lvs",
"--help", NULL);
+  if (r == -1) {
+    reply_with_error ("lvm lvs --help: %s", err);
+    return -1;
+  }
+
+  if (strstr (out, "-S") == NULL)
+    result = 0;
+  else
+    result = 1;
+
+  return result;
+}
+
 char **
 do_lvs (void)
 {
   char *out;
   CLEANUP_FREE char *err = NULL;
   int r;
+  int has_S = test_lvs_has_S_opt ();
 
-  r = command (&out, &err,
-               str_lvm, "lvs",
-               "-o", "vg_name,lv_name",
-               "-S", "lv_role=public",
-               "--noheadings",
-               "--separator", "/", NULL);
-  if (r == -1) {
-    reply_with_error ("%s", err);
-    free (out);
+  if (has_S < 0)
     return NULL;
-  }
 
-  return convert_lvm_output (out, "/dev/");
+  if (has_S > 0) {
+    r = command (&out, &err,
+                 str_lvm, "lvs",
+                 "-o", "vg_name,lv_name",
+                 "-S", "lv_role=public",
+                 "--noheadings",
+                 "--separator", "/", NULL);
+    if (r == -1) {
+      reply_with_error ("%s", err);
+      free (out);
+      return NULL;
+    }
+
+    return convert_lvm_output (out, "/dev/");
+  } else {
+    r = command (&out, &err,
+                 str_lvm, "lvs",
+                 "-o", "lv_attr,vg_name,lv_name",
+                 "--noheadings",
+                 "--separator", ":", NULL);
+    if (r == -1) {
+      reply_with_error ("%s", err);
+      free (out);
+      return NULL;
+    }
+
+    return filter_convert_old_lvs_output (out);
+  }
 }
 
 /* These were so complex to implement that I ended up auto-generating
-- 
2.5.0

On Thu, Jan 28, 2016 at 04:11:30PM +0100, Pino Toscano
wrote:
> lvm2 2.02.107 adds the -S/--select option used in lvs to filter out only
> public LVs (see RHBZ#1278878).  To make this work again with versions
> of lvm2 older than that, only on old versions filter out thin layouts
> and compose the resulting device strings ourselves.
> 
> The filtering done is much simplier than what "-S lv_role=public"
will
> do, but should be good enough for our need.
> ---
>  daemon/lvm.c | 147
++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 136 insertions(+), 11 deletions(-)
> 
> diff --git a/daemon/lvm.c b/daemon/lvm.c
> index 979cf63..8bef4d5 100644
> --- a/daemon/lvm.c
> +++ b/daemon/lvm.c
> @@ -103,6 +103,85 @@ convert_lvm_output (char *out, const char *prefix)
>    return ret.argv;
>  }
>  
> +/* Filter a colon-separated output of
> + *   lvs -o lv_attr,vg_name,lv_name
> + * removing thin layouts, and building the device path as we expect it.
> + *
> + * This is used only when lvm has no -S.
> + */
> +static char **
> +filter_convert_old_lvs_output (char *out)
> +{
> +  char *p, *pend;
> +  DECLARE_STRINGSBUF (ret);
> +
> +  p = out;
> +  while (p) {
> +    size_t len;
> +    char *saveptr;
> +    char *lv_attr, *vg_name, *lv_name;
> +
> +    pend = strchr (p, '\n');	/* Get the next line of output. */
> +    if (pend) {
> +      *pend = '\0';
> +      pend++;
> +    }
> +
> +    while (*p && c_isspace (*p))	/* Skip any leading whitespace.
*/
> +      p++;
> +
> +    /* Sigh, skip trailing whitespace too.  "pvs", I'm
looking at you. */
> +    len = strlen (p)-1;
> +    while (*p && c_isspace (p[len]))
> +      p[len--] = '\0';
> +
> +    if (!*p) {			/* Empty line?  Skip it. */
> +    skip_line:
> +      p = pend;
> +      continue;
> +    }
> +
> +    lv_attr = strtok_r (p, ":", &saveptr);
> +    if (!lv_attr)
> +      goto skip_line;
> +
> +    vg_name = strtok_r (NULL, ":", &saveptr);
> +    if (!vg_name)
> +      goto skip_line;
> +
> +    lv_name = strtok_r (NULL, ":", &saveptr);
> +    if (!lv_name)
> +      goto skip_line;
> +
> +    /* Ignore thin layouts (RHBZ#1278878). */
> +    if (lv_attr[0] == 't')
> +      goto skip_line;
> +
> +    /* Ignore "unknown device" message (RHBZ#1054761). */
> +    if (STRNEQ (p, "unknown device")) {
> +      char buf[256];
> +
> +      snprintf (buf, sizeof buf, "/dev/%s/%s", vg_name,
lv_name);
> +      if (add_string (&ret, buf) == -1) {
> +        free (out);
> +        return NULL;
> +      }
> +    }
> +
> +    p = pend;
> +  }
> +
> +  free (out);
> +
> +  if (ret.size > 0)
> +    sort_strings (ret.argv, ret.size);
> +
> +  if (end_stringsbuf (&ret) == -1)
> +    return NULL;
> +
> +  return ret.argv;
> +}
> +
>  char **
>  do_pvs (void)
>  {
> @@ -139,26 +218,72 @@ do_vgs (void)
>    return convert_lvm_output (out, NULL);
>  }
>  
> +/* Check whether lvs has -S to filter its output.
> + * It is available only in lvm2 >= 2.02.107.
> + */
> +static int
> +test_lvs_has_S_opt (void)
> +{
> +  static int result = -1;
> +  if (result != -1)
> +    return result;
> +
> +  CLEANUP_FREE char *out = NULL;
> +  CLEANUP_FREE char *err = NULL;
> +
> +  int r = command (&out, &err, str_lvm, "lvs",
"--help", NULL);
> +  if (r == -1) {
> +    reply_with_error ("lvm lvs --help: %s", err);
> +    return -1;
> +  }
> +
> +  if (strstr (out, "-S") == NULL)
> +    result = 0;
> +  else
> +    result = 1;
> +
> +  return result;
> +}
> +
>  char **
>  do_lvs (void)
>  {
>    char *out;
>    CLEANUP_FREE char *err = NULL;
>    int r;
> +  int has_S = test_lvs_has_S_opt ();
>  
> -  r = command (&out, &err,
> -               str_lvm, "lvs",
> -               "-o", "vg_name,lv_name",
> -               "-S", "lv_role=public",
> -               "--noheadings",
> -               "--separator", "/", NULL);
> -  if (r == -1) {
> -    reply_with_error ("%s", err);
> -    free (out);
> +  if (has_S < 0)
>      return NULL;
> -  }
>  
> -  return convert_lvm_output (out, "/dev/");
> +  if (has_S > 0) {
> +    r = command (&out, &err,
> +                 str_lvm, "lvs",
> +                 "-o", "vg_name,lv_name",
> +                 "-S", "lv_role=public",
> +                 "--noheadings",
> +                 "--separator", "/", NULL);
> +    if (r == -1) {
> +      reply_with_error ("%s", err);
> +      free (out);
> +      return NULL;
> +    }
> +
> +    return convert_lvm_output (out, "/dev/");
> +  } else {
> +    r = command (&out, &err,
> +                 str_lvm, "lvs",
> +                 "-o", "lv_attr,vg_name,lv_name",
> +                 "--noheadings",
> +                 "--separator", ":", NULL);
> +    if (r == -1) {
> +      reply_with_error ("%s", err);
> +      free (out);
> +      return NULL;
> +    }
> +
> +    return filter_convert_old_lvs_output (out);
> +  }
>  }
ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v