[PATCH] use vgs --nameprefixes to protect against whitespace changes

[PATCH] use vgs --nameprefixes to protect against whitespace changes

[Andrey Borzenkov]

Do not assume single blank at the end of PV name. At least
lvm2-2.02.98-24.3 (openSUSE 13.1 M3) puts exra blank at the end.

linux-chxo:~ # grub2-probe -t abstraction /lib
mdadm: cannot open /dev/md1  : No such file or directory
grub2-probe: error: cannot open `/dev/md1  ': No such file or directory.
linux-chxo:~ # : Update grub with patch
linux-chxo:~ # grub2-probe -t abstraction /lib
diskfilter mdraid1x lvm 
linux-chxo:~ # vgs --options pv_name --noheadings | xxd
0000000: 2020 2f64 6576 2f6d 6431 2020 0a           /dev/md1  .
linux-chxo:~ # rpm -q lvm2
lvm2-2.02.98-24.3.i586

This patch changes it to use --nameprefixes which returns each PV name as
  LVM2_PV_NAME='xxx'

Signed-off-by: Andrey Borzenkov <arvidjaar@gmail.com>

---
 util/getroot.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/util/getroot.c b/util/getroot.c
index 2ad8a55..479ab78 100644
--- a/util/getroot.c
+++ b/util/getroot.c
@@ -1322,7 +1322,7 @@ grub_util_get_dev_abstraction (const char *os_dev)
 static void
 pull_lvm_by_command (const char *os_dev)
 {
-  char *argv[6];
+  char *argv[7];
   int fd;
   pid_t pid;
   FILE *mdadm;
@@ -1355,8 +1355,9 @@ pull_lvm_by_command (const char *os_dev)
   argv[1] = (char *) "--options";
   argv[2] = (char *) "pv_name";
   argv[3] = (char *) "--noheadings";
-  argv[4] = vgname;
-  argv[5] = NULL;
+  argv[4] = (char *) "--nameprefixes";
+  argv[5] = vgname;
+  argv[6] = NULL;
 
   pid = exec_pipe (argv, &fd);
   free (vgname);
@@ -1376,10 +1377,19 @@ pull_lvm_by_command (const char *os_dev)
   while (getline (&buf, &len, mdadm) > 0)
     {
       char *ptr;
-      for (ptr = buf; ptr < buf + 2 && *ptr == ' '; ptr++);
+
+      grub_util_info ("vgs returned PV: %s", buf);
+
+      for (ptr = buf; *ptr == ' '; ptr++);
       if (*ptr == '\0')
 	continue;
-      *(ptr + strlen (ptr) - 1) = '\0';
+      if (strncmp (buf, "LVM2_PV_NAME='", sizeof ("LVM2_PV_NAME='") - 1))
+	continue;
+      ptr = buf + sizeof ("LVM2_PV_NAME='") - 1;
+      if (*(buf + len - 1) != '\'')
+	continue;
+      *(buf + len - 1) = '\0';
+      /* FIXME can PV name contain "'"? */
       grub_util_pull_device (ptr);
     }
 
-- 
tg: (e1a892d..) u/strip-pv-trailing-blanks (depends on: master)

[PATCH] fix parsing of LVM PV names for short names

[Andrey Borzenkov]

Default format of vgs output is

- two spaces as standard prefix
- PV name left aligned to field width which is 10 characters

This means that if PV name has less than 10 chacaters it has some spaces at
the end.

Example:

linux-chxo:~ # vgs -o pv_name --noheadings | cat -E
  /dev/md1  $
  /dev/md101$
linux-chxo:~ # 

There is no explicit option to turn off alignment; it is implicitly
disabled if one of --separator or --nameprefixes option is used.

--separator was added in 2007, --nameprefixes - in 2009. So let's use
--separator to extend range of versions we are compatible with. Note that
one or another must be used, current parsing is broken otherwise.

Signed-off-by: Andrey Borzenkov <arvidjaar@gmail.com>

---
 util/getroot.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/util/getroot.c b/util/getroot.c
index 2ad8a55..3afcf96 100644
--- a/util/getroot.c
+++ b/util/getroot.c
@@ -1322,7 +1322,7 @@ grub_util_get_dev_abstraction (const char *os_dev)
 static void
 pull_lvm_by_command (const char *os_dev)
 {
-  char *argv[6];
+  char *argv[8];
   int fd;
   pid_t pid;
   FILE *mdadm;
@@ -1351,12 +1351,17 @@ pull_lvm_by_command (const char *os_dev)
 
   /* execvp has inconvenient types, hence the casts.  None of these
      strings will actually be modified.  */
+  /* by default PV name is left aligned in 10 character field, meaning that
+     we do not know where name ends. Using dummy --separator disables
+     alignment. We have a single field, so separator itself is not output */
   argv[0] = (char *) "vgs";
   argv[1] = (char *) "--options";
   argv[2] = (char *) "pv_name";
   argv[3] = (char *) "--noheadings";
-  argv[4] = vgname;
-  argv[5] = NULL;
+  argv[4] = (char *) "--separator";
+  argv[5] = (char *) ":";
+  argv[6] = vgname;
+  argv[7] = NULL;
 
   pid = exec_pipe (argv, &fd);
   free (vgname);
@@ -1376,6 +1381,7 @@ pull_lvm_by_command (const char *os_dev)
   while (getline (&buf, &len, mdadm) > 0)
     {
       char *ptr;
+      /* LVM adds two spaces as standard prefix */
       for (ptr = buf; ptr < buf + 2 && *ptr == ' '; ptr++);
       if (*ptr == '\0')
 	continue;
-- 
tg: (ebd40b6..) u/strip-pv-trailing-blanks (depends on: master)

Re: [PATCH] fix parsing of LVM PV names for short names

[Vladimir 'φ-coder/phcoder' Serbinenko]

Go ahead
On 27.07.2013 12:09, Andrey Borzenkov wrote:
> Default format of vgs output is
>
> - two spaces as standard prefix
> - PV name left aligned to field width which is 10 characters
>
> This means that if PV name has less than 10 chacaters it has some spaces at
> the end.
>
> Example:
>
> linux-chxo:~ # vgs -o pv_name --noheadings | cat -E
>    /dev/md1  $
>    /dev/md101$
> linux-chxo:~ #
>
> There is no explicit option to turn off alignment; it is implicitly
> disabled if one of --separator or --nameprefixes option is used.
>
> --separator was added in 2007, --nameprefixes - in 2009. So let's use
> --separator to extend range of versions we are compatible with. Note that
> one or another must be used, current parsing is broken otherwise.
>
> Signed-off-by: Andrey Borzenkov <arvidjaar@gmail.com>
>
> ---
>   util/getroot.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/util/getroot.c b/util/getroot.c
> index 2ad8a55..3afcf96 100644
> --- a/util/getroot.c
> +++ b/util/getroot.c
> @@ -1322,7 +1322,7 @@ grub_util_get_dev_abstraction (const char *os_dev)
>   static void
>   pull_lvm_by_command (const char *os_dev)
>   {
> -  char *argv[6];
> +  char *argv[8];
>     int fd;
>     pid_t pid;
>     FILE *mdadm;
> @@ -1351,12 +1351,17 @@ pull_lvm_by_command (const char *os_dev)
>
>     /* execvp has inconvenient types, hence the casts.  None of these
>        strings will actually be modified.  */
> +  /* by default PV name is left aligned in 10 character field, meaning that
> +     we do not know where name ends. Using dummy --separator disables
> +     alignment. We have a single field, so separator itself is not output */
>     argv[0] = (char *) "vgs";
>     argv[1] = (char *) "--options";
>     argv[2] = (char *) "pv_name";
>     argv[3] = (char *) "--noheadings";
> -  argv[4] = vgname;
> -  argv[5] = NULL;
> +  argv[4] = (char *) "--separator";
> +  argv[5] = (char *) ":";
> +  argv[6] = vgname;
> +  argv[7] = NULL;
>
>     pid = exec_pipe (argv, &fd);
>     free (vgname);
> @@ -1376,6 +1381,7 @@ pull_lvm_by_command (const char *os_dev)
>     while (getline (&buf, &len, mdadm) > 0)
>       {
>         char *ptr;
> +      /* LVM adds two spaces as standard prefix */
>         for (ptr = buf; ptr < buf + 2 && *ptr == ' '; ptr++);
>         if (*ptr == '\0')
>   	continue;
>

Re: [PATCH] use vgs --nameprefixes to protect against whitespace changes

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 19.07.2013 09:11, Andrey Borzenkov wrote:
> Do not assume single blank at the end of PV name. At least
> lvm2-2.02.98-24.3 (openSUSE 13.1 M3) puts exra blank at the end.
>
> linux-chxo:~ # grub2-probe -t abstraction /lib
> mdadm: cannot open /dev/md1  : No such file or directory
> grub2-probe: error: cannot open `/dev/md1  ': No such file or directory.
> linux-chxo:~ # : Update grub with patch
> linux-chxo:~ # grub2-probe -t abstraction /lib
> diskfilter mdraid1x lvm
> linux-chxo:~ # vgs --options pv_name --noheadings | xxd
> 0000000: 2020 2f64 6576 2f6d 6431 2020 0a           /dev/md1  .
> linux-chxo:~ # rpm -q lvm2
> lvm2-2.02.98-24.3.i586
>
> This patch changes it to use --nameprefixes which returns each PV name as
>    LVM2_PV_NAME='xxx'
>
This code wouldn't handle name containing \n correctly but it's a 
subject for another patch as issue is already present.
Go ahead.
> Signed-off-by: Andrey Borzenkov <arvidjaar@gmail.com>
>
> ---
>   util/getroot.c | 20 +++++++++++++++-----
>   1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/util/getroot.c b/util/getroot.c
> index 2ad8a55..479ab78 100644
> --- a/util/getroot.c
> +++ b/util/getroot.c
> @@ -1322,7 +1322,7 @@ grub_util_get_dev_abstraction (const char *os_dev)
>   static void
>   pull_lvm_by_command (const char *os_dev)
>   {
> -  char *argv[6];
> +  char *argv[7];
>     int fd;
>     pid_t pid;
>     FILE *mdadm;
> @@ -1355,8 +1355,9 @@ pull_lvm_by_command (const char *os_dev)
>     argv[1] = (char *) "--options";
>     argv[2] = (char *) "pv_name";
>     argv[3] = (char *) "--noheadings";
> -  argv[4] = vgname;
> -  argv[5] = NULL;
> +  argv[4] = (char *) "--nameprefixes";
> +  argv[5] = vgname;
> +  argv[6] = NULL;
>
>     pid = exec_pipe (argv, &fd);
>     free (vgname);
> @@ -1376,10 +1377,19 @@ pull_lvm_by_command (const char *os_dev)
>     while (getline (&buf, &len, mdadm) > 0)
>       {
>         char *ptr;
> -      for (ptr = buf; ptr < buf + 2 && *ptr == ' '; ptr++);
> +
> +      grub_util_info ("vgs returned PV: %s", buf);
> +
> +      for (ptr = buf; *ptr == ' '; ptr++);
>         if (*ptr == '\0')
>   	continue;
> -      *(ptr + strlen (ptr) - 1) = '\0';
> +      if (strncmp (buf, "LVM2_PV_NAME='", sizeof ("LVM2_PV_NAME='") - 1))
> +	continue;
> +      ptr = buf + sizeof ("LVM2_PV_NAME='") - 1;
> +      if (*(buf + len - 1) != '\'')
> +	continue;
> +      *(buf + len - 1) = '\0';
> +      /* FIXME can PV name contain "'"? */
>         grub_util_pull_device (ptr);
>       }
>
>