Re: Bug#494383: grub-pc: Grub2 cannot find LVM volume groups with a dash (-) in the name

Re: Bug#494383: grub-pc: Grub2 cannot find LVM volume groups with a dash (-) in the name

[Felix Zielcke]

Hello,

Am Freitag, den 08.08.2008, 22:23 +0200 schrieb root:

> grub-probe: error: Unknown device linux--vg-boot
> Autodetection of a file system module failed
> Please specify the module with the option "--modules" explicitly
> 

I never used LVM before so maybe I did something wrong,
but I think the lvm2 Debian package has a bug:

fz-vm:~# pvcreate /dev/sdb
  Physical volume "/dev/sdb" successfully created
fz-vm:~# vgcreate linux-vg /dev/sdb
  Volume group "linux-vg" successfully created
fz-vm:~# lvcreate -L 1M linux-vg 
  Rounding up size to full physical extent 4,00 MB
  Logical volume "lvol0" created
fz-vm:~# ls /dev/mapper/
control  linux--vg-lvol0

Re: Bug#494383: grub-pc: Grub2 cannot find LVM volume groups with a dash (-) in the name

[Felix Zielcke]

Hi,

Am Samstag, den 09.08.2008, 12:26 +0200 schrieb Felix Zielcke:

> fz-vm:~# ls /dev/mapper/
> control  linux--vg-lvol0
> 
And again it wasn't that clear with my first mail :(

fz-vm:~# l /dev/linux* /dev/mapper/
/dev/linux-vg:
insgesamt 0
lrwxrwxrwx 1 root root 27  9. Aug 14:49 lvol0
-> /dev/mapper/linux--vg-lvol0

/dev/linuxvg:
insgesamt 0
lrwxrwxrwx 1 root root 25  9. Aug 14:49 lvol0
-> /dev/mapper/linuxvg-lvol0

/dev/mapper/:
insgesamt 0
crw-rw---- 1 root root  10, 60  9. Aug 14:49 control
brw-rw---- 1 root disk 253,  1  9. Aug 14:49 linux--vg-lvol0
brw-rw---- 1 root disk 253,  0  9. Aug 14:49 linuxvg-lvol0

Current upstream SVN shows me:

fz-vm:/dev/mapper# grub-probe /mnt/
error: no mapping exists for `linux--vg-lvol0'
grub-probe: error: no mapping exists for `linux--vg-lvol0'

If I change the double dash to one and mount it again then it works
fine.

This bug happens at least with recent udev/lvm2 from Debian unstable
with kernel.org 2.6.27-rc2

I failed to find out how to workaround this in GRUB and I'm a bit unsure
if we even should do this.
What happens if this is fixed in lvm2/udev/kernel (wherever it belongs
too) and what is with people who like to have 2 dashes in there vg name?

(Robert: In this LVM case I think it's not that bad that
debian-installer currently prefers LILO over GRUB 2 ;))

Re: Bug#494383: grub-pc: Grub2 cannot find LVM volume groups with a dash (-) in the name

[Sander Marechal]

Felix Zielcke wrote:
> Hello,
> 
> Am Freitag, den 08.08.2008, 22:23 +0200 schrieb root:
> 
>> grub-probe: error: Unknown device linux--vg-boot
>> Autodetection of a file system module failed
>> Please specify the module with the option "--modules" explicitly
>>
> 
> I never used LVM before so maybe I did something wrong,
> but I think the lvm2 Debian package has a bug:

That's not a bug. See for example this old Ubuntu bug:
https://bugs.launchpad.net/ubuntu/+source/initramfs-tools/+bug/19631

LVM and /dev/mapper keep names like this:

vgname-lvname

When there's a dash in either the vgname or the lvname, it is doubled to
excape it. So "my-volume-group" and "my-logical-volume" becomes:

my--volume--group-my--logical--volume

I think Grub2 should be able to cope with that.

-- 
Sander Marechal

[PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 920 bytes --]

I still can't see any use in this `escape a dash with a dash because we
use a dash to seperate the vg part from the lv one' from LVM.
As I already wrote, grub-probe works fine if you remove that double
escaping from the file name and remount it.
LVM (lv* vg* commands) shouldn't use the filename to get the vg and lv
part ...

fz-vm:/dev# lvdisplay
  --- Logical volume ---
  LV Name                /dev/m-y--vg/l-vol--0
  VG Name                m-y--vg

lvdisplay is even showing the unescaped directory + symlink

Well attached is now my cute little asprintf patch with a very ugly LVM
part.

It works for

volumegroup: linuxvg
logicalvolume: lvol0
/dev/mapper/linuxvg-lvol0

volume group: m-y--vg
logical volume: l-vol--0
/dev/mapper/m--y----vg-l--vol----0


Please comment this, please give me hints how to make it better.
I don't like the code myself but it seems like we should handle this
double dash problem :(


[-- Attachment #2: getroot.diff --]
[-- Type: text/x-patch, Size: 2864 bytes --]

Index: util/getroot.c
===================================================================
--- util/getroot.c	(Revision 1799)
+++ util/getroot.c	(Arbeitskopie)
@@ -17,6 +17,7 @@
  *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <config.h>
 #include <sys/stat.h>
 #include <unistd.h>
 #include <string.h>
@@ -406,67 +407,56 @@
 
   switch (grub_util_get_dev_abstraction (os_dev))
     {
-    case GRUB_DEV_ABSTRACTION_LVM:
-      grub_dev = xmalloc (strlen (os_dev) - 12 + 1);
-
-      strcpy (grub_dev, os_dev + 12);
-
-      break;
-
-    case GRUB_DEV_ABSTRACTION_RAID:
-      grub_dev = xmalloc (20);
-
-      if (os_dev[7] == '_' && os_dev[8] == 'd')
+      case GRUB_DEV_ABSTRACTION_LVM:
+	
 	{
-	  const char *p;
+	  unsigned char i, j, k, l;
 
-	  /* This a partitionable RAID device of the form /dev/md_dNNpMM. */
-	  int i;
+	  j = sizeof ("/dev/mapper/") -1;
+	  l = strlen (os_dev) - j + 1;
 
-	  grub_dev[0] = 'm';
-	  grub_dev[1] = 'd';
-	  i = 2;
-	  
-	  p = os_dev + 9;
-	  while (*p >= '0' && *p <= '9')
+	  grub_dev = xmalloc (strlen (os_dev) - strlen ("/dev/mapper/") + 1);
+	
+	  for (i = 0, k = 0; i < l; i++)
 	    {
-	      grub_dev[i] = *p;
-	      i++;
-	      p++;
+	       grub_dev[k] = os_dev[j + i];
+	       k++;
+	       if (os_dev[j + i] == '-' && os_dev[j + i + 1] == '-')
+		 i++;
 	    }
+	}
 
-	  if (*p == '\0')
-	    grub_dev[i] = '\0';
-	  else if (*p == 'p')
-	    {
-	      p++;
-	      grub_dev[i] = ',';
-	      i++;
+      break;
 
-	      while (*p >= '0' && *p <= '9')
-		{
-		  grub_dev[i] = *p;
-		  i++;
-		  p++;
-		}
+      case GRUB_DEV_ABSTRACTION_RAID:
 
-	      grub_dev[i] = '\0';
-	    }
-	  else
-	    grub_util_error ("Unknown kind of RAID device `%s'", os_dev);
-	}
-      else if (os_dev[7] >= '0' && os_dev[7] <= '9')
-	{
-	  memcpy (grub_dev, os_dev + 5, 7);
-	  grub_dev[7] = '\0';
-	}
-      else
-	grub_util_error ("Unknown kind of RAID device `%s'", os_dev);
+	if (os_dev[7] == '_' && os_dev[8] == 'd')
+	  {
+	    /* This a partitionable RAID device of the form /dev/md_dNNpMM. */
 
-      break;
+	    char *p;
 
-    default:  /* GRUB_DEV_ABSTRACTION_NONE */
-      grub_dev = grub_util_biosdisk_get_grub_dev (os_dev);
+	    p = strchr (os_dev, 'p');
+	    if (p)
+	      *p = ',';
+
+	    asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md_d") - 1);
+	  }
+	else if (os_dev[7] >= '0' && os_dev[7] <= '9')
+	  {
+	    asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md") - 1);
+	  }
+	else if (os_dev[7] == '/' && os_dev[8] >= '0' && os_dev[8] <= '9')
+	  {
+	    asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md/") - 1);
+	  }
+	else
+	  grub_util_error ("Unknown kind of RAID device `%s'", os_dev);
+
+	break;
+
+      default:  /* GRUB_DEV_ABSTRACTION_NONE */
+	grub_dev = grub_util_biosdisk_get_grub_dev (os_dev);
     }
 
   return grub_dev;

Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name

[Sander Marechal]

Felix Zielcke wrote:
> Well attached is now my cute little asprintf patch with a very ugly LVM
> part.
> 
> It works for
> 
> volumegroup: linuxvg
> logicalvolume: lvol0
> /dev/mapper/linuxvg-lvol0
> 
> volume group: m-y--vg
> logical volume: l-vol--0
> /dev/mapper/m--y----vg-l--vol----0
> 
> 
> Please comment this, please give me hints how to make it better.
> I don't like the code myself but it seems like we should handle this
> double dash problem :(

I commented on this on #grub but will post here as well, to preserve it
for posterity :-)

I tested the patch against grub 20080724 in Debian Lenny and it works
great :-) I can't comment on the rest of the code. I'm not a grub developer.

-- 
Sander Marechal

Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 948 bytes --]

Am Montag, den 11.08.2008, 18:18 +0200 schrieb Felix Zielcke:
> 
> Please comment this, please give me hints how to make it better.
> I don't like the code myself but it seems like we should handle this
> double dash problem :(
> 

Now that a bit time has passed, I think it isn't that ugly.
But I still don't like it.
I still haven't yet an idea how to make it better, I don't think it's
that great to introduce 4 variables just to remove every 2nd dash.

Unfortunately nobody commented on this new LVM part.

Idention was still not that right I think and I moved the `grub_dev =
xmalloc ()' line above the 2 `j = ..' and `l = ..'.

Marco, I know that you think this is a bit lazy, but I think mentioning
the double dash escaping and that it now uses asprintf for the RAID
cases would be a bit too verbose.

2008-08-14  Felix Zielcke  <fzielcke@z-51.de>

        * util/getroot.c: Include <config.h>.
        (grub_util_get_grub_dev): Rewritten.


[-- Attachment #2: getroot.diff --]
[-- Type: text/x-patch, Size: 2900 bytes --]

Index: util/getroot.c
===================================================================
--- util/getroot.c	(Revision 1804)
+++ util/getroot.c	(Arbeitskopie)
@@ -17,6 +17,7 @@
  *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <config.h>
 #include <sys/stat.h>
 #include <unistd.h>
 #include <string.h>
@@ -406,67 +407,56 @@ grub_util_get_grub_dev (const char *os_d
 
   switch (grub_util_get_dev_abstraction (os_dev))
     {
-    case GRUB_DEV_ABSTRACTION_LVM:
-      grub_dev = xmalloc (strlen (os_dev) - 12 + 1);
+      case GRUB_DEV_ABSTRACTION_LVM:
 
-      strcpy (grub_dev, os_dev + 12);
+ 	{
+	  unsigned char i, j, k, l;
 
-      break;
+	  grub_dev = xmalloc (strlen (os_dev) - strlen ("/dev/mapper/") + 1);
 
-    case GRUB_DEV_ABSTRACTION_RAID:
-      grub_dev = xmalloc (20);
-
-      if (os_dev[7] == '_' && os_dev[8] == 'd')
-	{
-	  const char *p;
-
-	  /* This a partitionable RAID device of the form /dev/md_dNNpMM. */
-	  int i;
-
-	  grub_dev[0] = 'm';
-	  grub_dev[1] = 'd';
-	  i = 2;
-	  
-	  p = os_dev + 9;
-	  while (*p >= '0' && *p <= '9')
+	  j = sizeof ("/dev/mapper/") -1;
+	  l = strlen (os_dev) - j + 1;
+	
+	  for (i = 0, k = 0; i < l; i++)
 	    {
-	      grub_dev[i] = *p;
-	      i++;
-	      p++;
+	      grub_dev[k] = os_dev[j + i];
+	      k++;
+	      if (os_dev[j + i] == '-' && os_dev[j + i + 1] == '-')
+		i++;
 	    }
+	}
 
-	  if (*p == '\0')
-	    grub_dev[i] = '\0';
-	  else if (*p == 'p')
-	    {
-	      p++;
-	      grub_dev[i] = ',';
-	      i++;
-
-	      while (*p >= '0' && *p <= '9')
-		{
-		  grub_dev[i] = *p;
-		  i++;
-		  p++;
-		}
+	break;
 
-	      grub_dev[i] = '\0';
-	    }
-	  else
-	    grub_util_error ("Unknown kind of RAID device `%s'", os_dev);
-	}
-      else if (os_dev[7] >= '0' && os_dev[7] <= '9')
-	{
-	  memcpy (grub_dev, os_dev + 5, 7);
-	  grub_dev[7] = '\0';
-	}
-      else
-	grub_util_error ("Unknown kind of RAID device `%s'", os_dev);
+      case GRUB_DEV_ABSTRACTION_RAID:
+
+	if (os_dev[7] == '_' && os_dev[8] == 'd')
+	  {
+	    /* This a partitionable RAID device of the form /dev/md_dNNpMM. */
+
+	    char *p;
+
+	    p = strchr (os_dev, 'p');
+	    if (p)
+	      *p = ',';
+
+	    asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md_d") - 1);
+	  }
+	else if (os_dev[7] >= '0' && os_dev[7] <= '9')
+	  {
+	    asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md") - 1);
+	  }
+	else if (os_dev[7] == '/' && os_dev[8] >= '0' && os_dev[8] <= '9')
+	  {
+	    asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md/") - 1);
+	  }
+	else
+	  grub_util_error ("Unknown kind of RAID device `%s'", os_dev);
 
-      break;
+	break;
 
-    default:  /* GRUB_DEV_ABSTRACTION_NONE */
-      grub_dev = grub_util_biosdisk_get_grub_dev (os_dev);
+      default:  /* GRUB_DEV_ABSTRACTION_NONE */
+	grub_dev = grub_util_biosdisk_get_grub_dev (os_dev);
     }
 
   return grub_dev;

Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name

[Robert Millan]

On Thu, Aug 14, 2008 at 07:34:05PM +0200, Felix Zielcke wrote:
> 2008-08-14  Felix Zielcke  <fzielcke@z-51.de>
> 
>         * util/getroot.c: Include <config.h>.
>         (grub_util_get_grub_dev): Rewritten.

Please explain the actual changes (or what they're for).

>    switch (grub_util_get_dev_abstraction (os_dev))
>      {
> -    case GRUB_DEV_ABSTRACTION_LVM:
> -      grub_dev = xmalloc (strlen (os_dev) - 12 + 1);
> +      case GRUB_DEV_ABSTRACTION_LVM:

Try to ommit big indentation changes from the patch.  If you change something
that requires a big chunk of code to be reindented, I think it's better if you
leave that out for readability (and mention so if you like).

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name

[Felix Zielcke]

Am Donnerstag, den 14.08.2008, 20:03 +0200 schrieb Robert Millan:
> 
> Please explain the actual changes (or what they're for).
As I wrote that, I just saw before that Marco did just Rewritten. in a
commit, but with searching for Rewritten in the ChangeLog reveals much
more verbose entrys

So:

2008-08-14  Felix Zielcke  <fzielcke@z-51.de>

        * util/getroot.c: Include <config.h>.
        (grub_util_get_grub_dev): Rewritten to use asprintf for mdraid devices, added
        support for /dev/md/N devices and handles LVM double dash escaping.

> 
> >    switch (grub_util_get_dev_abstraction (os_dev))
> >      {
> > -    case GRUB_DEV_ABSTRACTION_LVM:
> > -      grub_dev = xmalloc (strlen (os_dev) - 12 + 1);
> > +      case GRUB_DEV_ABSTRACTION_LVM:
> 
> Try to ommit big indentation changes from the patch.  If you change something
> that requires a big chunk of code to be reindented, I think it's better if you
> leave that out for readability (and mention so if you like).

Unfortunately I couldn't get the -w option working to ignore white space
changes
svn diff --diff-cmd diff -x -up -x -w doestn't produce an unified diff,
< and > are used instead of - and +
with -x -w -x up the -w is ignored :(

So I just need to remember this now that I either change the indention,
before commiting or I directly make 2 patches.
Luckly I can commit this now myself so I really don't need to include
this in the patches I send :)

Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name

[Robert Millan]

On Thu, Aug 14, 2008 at 09:32:04PM +0200, Felix Zielcke wrote:
> > Try to ommit big indentation changes from the patch.  If you change something
> > that requires a big chunk of code to be reindented, I think it's better if you
> > leave that out for readability (and mention so if you like).
> 
> Unfortunately I couldn't get the -w option working to ignore white space
> changes
> svn diff --diff-cmd diff -x -up -x -w doestn't produce an unified diff,
> < and > are used instead of - and +
> with -x -w -x up the -w is ignored :(
> 
> So I just need to remember this now that I either change the indention,
> before commiting or I directly make 2 patches.
> Luckly I can commit this now myself so I really don't need to include
> this in the patches I send :)

Okay, but please send the patch without indentation first, so your changes can
be reviewed.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 429 bytes --]

Am Donnerstag, den 14.08.2008, 22:38 +0200 schrieb Robert Millan:

> Okay, but please send the patch without indentation first, so your changes can
> be reviewed.


svn diff --diff-cmd diff -x -upw

That way it works, the attached getroot_w.diff is made with it.
But I think it doestn't look that good to review it, with wrong
indentation.
So I made now the getroot.diff new without indenting the whole switch
block by 2 spaces.

[-- Attachment #2: getroot.diff --]
[-- Type: text/x-patch, Size: 2289 bytes --]

Index: util/getroot.c
===================================================================
--- util/getroot.c	(Revision 1807)
+++ util/getroot.c	(Arbeitskopie)
@@ -17,6 +17,7 @@
  *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <config.h>
 #include <sys/stat.h>
 #include <unistd.h>
 #include <string.h>
@@ -407,58 +408,47 @@ grub_util_get_grub_dev (const char *os_d
   switch (grub_util_get_dev_abstraction (os_dev))
     {
     case GRUB_DEV_ABSTRACTION_LVM:
-      grub_dev = xmalloc (strlen (os_dev) - 12 + 1);
 
-      strcpy (grub_dev, os_dev + 12);
+      {
+	unsigned char i, j, k, l;
 
-      break;
+	grub_dev = xmalloc (strlen (os_dev) - strlen ("/dev/mapper/") + 1);
+
+	j = sizeof ("/dev/mapper/") -1;
+	l = strlen (os_dev) - j + 1;
+	  
+	for (i = 0, k = 0; i < l; i++)
+	  {
+	    grub_dev[k] = os_dev[j + i];
+	    k++;
+	    if (os_dev[j + i] == '-' && os_dev[j + i + 1] == '-')
+	      i++;
+	  }
+      }
+
+    break;
 
     case GRUB_DEV_ABSTRACTION_RAID:
-      grub_dev = xmalloc (20);
 
       if (os_dev[7] == '_' && os_dev[8] == 'd')
 	{
-	  const char *p;
-
 	  /* This a partitionable RAID device of the form /dev/md_dNNpMM. */
-	  int i;
-
-	  grub_dev[0] = 'm';
-	  grub_dev[1] = 'd';
-	  i = 2;
-	  
-	  p = os_dev + 9;
-	  while (*p >= '0' && *p <= '9')
-	    {
-	      grub_dev[i] = *p;
-	      i++;
-	      p++;
-	    }
 
-	  if (*p == '\0')
-	    grub_dev[i] = '\0';
-	  else if (*p == 'p')
-	    {
-	      p++;
-	      grub_dev[i] = ',';
-	      i++;
+	  char *p;
 
-	      while (*p >= '0' && *p <= '9')
-		{
-		  grub_dev[i] = *p;
-		  i++;
-		  p++;
-		}
+	  p = strchr (os_dev, 'p');
+	  if (p)
+	    *p = ',';
 
-	      grub_dev[i] = '\0';
-	    }
-	  else
-	    grub_util_error ("Unknown kind of RAID device `%s'", os_dev);
+	  asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md_d") - 1);
 	}
       else if (os_dev[7] >= '0' && os_dev[7] <= '9')
 	{
-	  memcpy (grub_dev, os_dev + 5, 7);
-	  grub_dev[7] = '\0';
+	  asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md") - 1);
+	}
+      else if (os_dev[7] == '/' && os_dev[8] >= '0' && os_dev[8] <= '9')
+	{
+	  asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md/") - 1);
 	}
       else
 	grub_util_error ("Unknown kind of RAID device `%s'", os_dev);

[-- Attachment #3: getroot_w.diff --]
[-- Type: text/x-patch, Size: 2433 bytes --]

Index: util/getroot.c
===================================================================
--- util/getroot.c	(Revision 1807)
+++ util/getroot.c	(Arbeitskopie)
@@ -17,6 +17,7 @@
  *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <config.h>
 #include <sys/stat.h>
 #include <unistd.h>
 #include <string.h>
@@ -407,58 +408,47 @@ grub_util_get_grub_dev (const char *os_d
   switch (grub_util_get_dev_abstraction (os_dev))
     {
     case GRUB_DEV_ABSTRACTION_LVM:
-      grub_dev = xmalloc (strlen (os_dev) - 12 + 1);
 
-      strcpy (grub_dev, os_dev + 12);
-
-      break;
-
-    case GRUB_DEV_ABSTRACTION_RAID:
-      grub_dev = xmalloc (20);
-
-      if (os_dev[7] == '_' && os_dev[8] == 'd')
 	{
-	  const char *p;
+	  unsigned char i, j, k, l;
 
-	  /* This a partitionable RAID device of the form /dev/md_dNNpMM. */
-	  int i;
+	  grub_dev = xmalloc (strlen (os_dev) - strlen ("/dev/mapper/") + 1);
 
-	  grub_dev[0] = 'm';
-	  grub_dev[1] = 'd';
-	  i = 2;
+	  j = sizeof ("/dev/mapper/") -1;
+	  l = strlen (os_dev) - j + 1;
 	  
-	  p = os_dev + 9;
-	  while (*p >= '0' && *p <= '9')
+	  for (i = 0, k = 0; i < l; i++)
 	    {
-	      grub_dev[i] = *p;
+	      grub_dev[k] = os_dev[j + i];
+	      k++;
+	      if (os_dev[j + i] == '-' && os_dev[j + i + 1] == '-')
 	      i++;
-	      p++;
+	    }
 	    }
 
-	  if (*p == '\0')
-	    grub_dev[i] = '\0';
-	  else if (*p == 'p')
-	    {
-	      p++;
-	      grub_dev[i] = ',';
-	      i++;
+	break;
 
-	      while (*p >= '0' && *p <= '9')
+      case GRUB_DEV_ABSTRACTION_RAID:
+
+	if (os_dev[7] == '_' && os_dev[8] == 'd')
 		{
-		  grub_dev[i] = *p;
-		  i++;
-		  p++;
-		}
+	    /* This a partitionable RAID device of the form /dev/md_dNNpMM. */
 
-	      grub_dev[i] = '\0';
-	    }
-	  else
-	    grub_util_error ("Unknown kind of RAID device `%s'", os_dev);
+	    char *p;
+
+	    p = strchr (os_dev, 'p');
+	    if (p)
+	      *p = ',';
+
+	    asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md_d") - 1);
 	}
       else if (os_dev[7] >= '0' && os_dev[7] <= '9')
 	{
-	  memcpy (grub_dev, os_dev + 5, 7);
-	  grub_dev[7] = '\0';
+	    asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md") - 1);
+	  }
+	else if (os_dev[7] == '/' && os_dev[8] >= '0' && os_dev[8] <= '9')
+	  {
+	    asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md/") - 1);
 	}
       else
 	grub_util_error ("Unknown kind of RAID device `%s'", os_dev);

Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 613 bytes --]

Am Donnerstag, den 14.08.2008, 23:15 +0200 schrieb Felix Zielcke:
> So I made now the getroot.diff new without indenting the whole switch
> block by 2 spaces.

Thanks to Marco and Emacs, I know now that the `case' on `switch' is a
special case :)
GCS isn't talking about this and I just assumed on seeing it that it's
wrong without bothering to look at other code :)

Thanks to Robert for the idea to make one of the variables a macro.

So the difference between this and last are:

I used Emacs instead of vi :D
j and k are swapped
new k is now #define
l is const

Changelog (in attached file) is still the same

[-- Attachment #2: getroot.diff --]
[-- Type: text/x-patch, Size: 2525 bytes --]

2008-08-14  Felix Zielcke  <fzielcke@z-51.de>

	* util/getroot.c: Include <config.h>.
	(grub_util_get_grub_dev): Rewritten to use asprintf for mdraid devices,
	added support for /dev/md/N devices and handles LVM double dash
	escaping.

Index: util/getroot.c
===================================================================
--- util/getroot.c	(Revision 1807)
+++ util/getroot.c	(Arbeitskopie)
@@ -17,6 +17,7 @@
  *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <config.h>
 #include <sys/stat.h>
 #include <unistd.h>
 #include <string.h>
@@ -407,58 +408,47 @@ grub_util_get_grub_dev (const char *os_d
   switch (grub_util_get_dev_abstraction (os_dev))
     {
     case GRUB_DEV_ABSTRACTION_LVM:
-      grub_dev = xmalloc (strlen (os_dev) - 12 + 1);
 
-      strcpy (grub_dev, os_dev + 12);
+      {
+	unsigned char i, j, k, l;
 
-      break;
+	grub_dev = xmalloc (strlen (os_dev) - strlen ("/dev/mapper/") + 1);
+
+	j = sizeof ("/dev/mapper/") -1;
+	l = strlen (os_dev) - j + 1;
+	  
+	for (i = 0, k = 0; i < l; i++)
+	  {
+	    grub_dev[k] = os_dev[j + i];
+	    k++;
+	    if (os_dev[j + i] == '-' && os_dev[j + i + 1] == '-')
+	      i++;
+	  }
+      }
+
+    break;
 
     case GRUB_DEV_ABSTRACTION_RAID:
-      grub_dev = xmalloc (20);
 
       if (os_dev[7] == '_' && os_dev[8] == 'd')
 	{
-	  const char *p;
-
 	  /* This a partitionable RAID device of the form /dev/md_dNNpMM. */
-	  int i;
-
-	  grub_dev[0] = 'm';
-	  grub_dev[1] = 'd';
-	  i = 2;
-	  
-	  p = os_dev + 9;
-	  while (*p >= '0' && *p <= '9')
-	    {
-	      grub_dev[i] = *p;
-	      i++;
-	      p++;
-	    }
 
-	  if (*p == '\0')
-	    grub_dev[i] = '\0';
-	  else if (*p == 'p')
-	    {
-	      p++;
-	      grub_dev[i] = ',';
-	      i++;
+	  char *p;
 
-	      while (*p >= '0' && *p <= '9')
-		{
-		  grub_dev[i] = *p;
-		  i++;
-		  p++;
-		}
+	  p = strchr (os_dev, 'p');
+	  if (p)
+	    *p = ',';
 
-	      grub_dev[i] = '\0';
-	    }
-	  else
-	    grub_util_error ("Unknown kind of RAID device `%s'", os_dev);
+	  asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md_d") - 1);
 	}
       else if (os_dev[7] >= '0' && os_dev[7] <= '9')
 	{
-	  memcpy (grub_dev, os_dev + 5, 7);
-	  grub_dev[7] = '\0';
+	  asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md") - 1);
+	}
+      else if (os_dev[7] == '/' && os_dev[8] >= '0' && os_dev[8] <= '9')
+	{
+	  asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md/") - 1);
 	}
       else
 	grub_util_error ("Unknown kind of RAID device `%s'", os_dev);

Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 869 bytes --]

Am Montag, den 18.08.2008, 17:05 +0200 schrieb Felix Zielcke:
> Am Donnerstag, den 14.08.2008, 23:15 +0200 schrieb Felix Zielcke:
> > So I made now the getroot.diff new without indenting the whole switch
> > block by 2 spaces.
> 
> Thanks to Marco and Emacs, I know now that the `case' on `switch' is a
> special case :)
> GCS isn't talking about this and I just assumed on seeing it that it's
> wrong without bothering to look at other code :)
> 
> Thanks to Robert for the idea to make one of the variables a macro.
> 
> So the difference between this and last are:
> 
> I used Emacs instead of vi :D
> j and k are swapped
> new k is now #define
> l is const
> 
> Changelog (in attached file) is still the same

/me screams load.
I even open the file again to put the changelog in it and then don't
notice that it's the old one :(
Well attached is now the right one.

[-- Attachment #2: getroot.diff --]
[-- Type: text/x-patch, Size: 2592 bytes --]

2008-08-14  Felix Zielcke  <fzielcke@z-51.de>

	* util/getroot.c: Include <config.h>.
	(grub_util_get_grub_dev): Rewritten to use asprintf for mdraid devices,
	added support for /dev/md/N devices and handles LVM double dash
	escaping.

Index: util/getroot.c
===================================================================
--- util/getroot.c	(Revision 1820)
+++ util/getroot.c	(Arbeitskopie)
@@ -17,6 +17,7 @@
  *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <config.h>
 #include <sys/stat.h>
 #include <unistd.h>
 #include <string.h>
@@ -407,58 +408,47 @@ grub_util_get_grub_dev (const char *os_d
   switch (grub_util_get_dev_abstraction (os_dev))
     {
     case GRUB_DEV_ABSTRACTION_LVM:
-      grub_dev = xmalloc (strlen (os_dev) - 12 + 1);
 
-      strcpy (grub_dev, os_dev + 12);
+      {
+#define k sizeof ("/dev/mapper/") - 1
 
-      break;
+	unsigned char i, j;
+	const unsigend char l = strlen (os_dev) - j + 1;
 
-    case GRUB_DEV_ABSTRACTION_RAID:
-      grub_dev = xmalloc (20);
+	grub_dev = xmalloc (strlen (os_dev) - strlen ("/dev/mapper/") + 1);
+	  
+	for (i = 0, j = 0; i < l; i++)
+	  {
+	    grub_dev[j] = os_dev[k + i];
+	    j++;
+	    if (os_dev[k + i] == '-' && os_dev[k + i + 1] == '-')
+	      i++;
+	  }
+      }
+
+    break;
 
+    case GRUB_DEV_ABSTRACTION_RAID:
+      
       if (os_dev[7] == '_' && os_dev[8] == 'd')
 	{
-	  const char *p;
-
 	  /* This a partitionable RAID device of the form /dev/md_dNNpMM. */
-	  int i;
-
-	  grub_dev[0] = 'm';
-	  grub_dev[1] = 'd';
-	  i = 2;
 	  
-	  p = os_dev + 9;
-	  while (*p >= '0' && *p <= '9')
-	    {
-	      grub_dev[i] = *p;
-	      i++;
-	      p++;
-	    }
-
-	  if (*p == '\0')
-	    grub_dev[i] = '\0';
-	  else if (*p == 'p')
-	    {
-	      p++;
-	      grub_dev[i] = ',';
-	      i++;
-
-	      while (*p >= '0' && *p <= '9')
-		{
-		  grub_dev[i] = *p;
-		  i++;
-		  p++;
-		}
+	  char *p;
+	  
+	  p = strchr (os_dev, 'p');
+	  if (p)
+	    *p = ',';
 
-	      grub_dev[i] = '\0';
-	    }
-	  else
-	    grub_util_error ("Unknown kind of RAID device `%s'", os_dev);
+	  asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md_d") - 1);
 	}
       else if (os_dev[7] >= '0' && os_dev[7] <= '9')
 	{
-	  memcpy (grub_dev, os_dev + 5, 7);
-	  grub_dev[7] = '\0';
+	  asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md") - 1);
+	}
+      else if (os_dev[7] == '/' && os_dev[8] >= '0' && os_dev[8] <= '9')
+	{
+	  asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md/") - 1);
 	}
       else
 	grub_util_error ("Unknown kind of RAID device `%s'", os_dev);

Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name

[Felix Zielcke]

Am Montag, den 18.08.2008, 17:16 +0200 schrieb Felix Zielcke:

> Well attached is now the right one.

I want to bring the topic up again, so that this hopefully, so that this
hopefully won't get forgotten for a while again.

I have just noticed that we have a regression in our new debian
experimental upload, i.e. this /dev/md/N and LVM dash thing isn't
included there because I just hoped that it will be commited first
upstream before we do the upload :(

-- 
Felix Zielcke

Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name

[Robert Millan]

On Mon, Aug 18, 2008 at 05:05:26PM +0200, Felix Zielcke wrote:
> +	unsigned char i, j, k, l;

I think using unsigned chars to store "integers" is counter-intuitive, and in
some cases possibly dangerous (overflow).

> +	grub_dev = xmalloc (strlen (os_dev) - strlen ("/dev/mapper/") + 1);
> +
> +	j = sizeof ("/dev/mapper/") -1;

                                     ^

Missing space here :-)

> +	for (i = 0, k = 0; i < l; i++)
> +	  {
> +	    grub_dev[k] = os_dev[j + i];
> +	    k++;

i already counts from 0 and increments by-one.  Can it be used instead of k?

The rest of the code I mostly don't understand well.  If you feel confident
that it's right, I suggest you check it in unless someone else also wants to
review it.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name

[Felix Zielcke]

Am Dienstag, den 02.09.2008, 00:14 +0200 schrieb Robert Millan:
> On Mon, Aug 18, 2008 at 05:05:26PM +0200, Felix Zielcke wrote:
> > +	unsigned char i, j, k, l;
> 
> I think using unsigned chars to store "integers" is counter-intuitive, and in
> some cases possibly dangerous (overflow).

I should have probable even used grub_uintN_t types.
16bit should be enough for them I think
255 chars in /dev/mapper/filename could be maybe a problem if people do
such weird things :)


> > +	grub_dev = xmalloc (strlen (os_dev) - strlen ("/dev/mapper/") + 1);
> > +
> > +	j = sizeof ("/dev/mapper/") -1;
> 
>                                      ^
> 
> Missing space here :-)

Yep and I even corrected this already in my last patch
http://lists.gnu.org/archive/html/grub-devel/2008-08/msg00523.htmlhttp://lists.gnu.org/archive/html/grub-devel/2008-08/msg00523.html

I'm now not that sure if that #define k would be okay, seems like Vesa
isn't liking macros.
But I could just use const for that too.

> > +	for (i = 0, k = 0; i < l; i++)
> > +	  {
> > +	    grub_dev[k] = os_dev[j + i];
> > +	    k++;
> 
> i already counts from 0 and increments by-one.  Can it be used instead of k?

Oh I just noticed again that in my last patch it's now j
i is used as the source count and get's incremented twice for a dash so
one dash is skipped.
whereas k (new j) is used as the destination count so it's always only
incremented by one.

That's the whole problem, skip the next dash on a dash but don't skip a
single dash.
It's always
/dev/mapper/vg-lv
but if vg and lv part has a single dash then it's for example
v--g-l--v
for grub this has to be v-g-l-v

> The rest of the code I mostly don't understand well.  If you feel confident
> that it's right, I suggest you check it in unless someone else also wants to
> review it.
> 

Honestly I'm happy that the LVM part seems to work and didn't introduce
any problem.

I'm too lazy now to make a new patch and go sleeping now.
But the changes I do make tomorrow now on the last patch above is
making k a const instead of #define, i think it just looks better
and using grub_uint16_t for all 4 variables.

I hope that Marco could have a quick look over it especially the
changelog part :)

-- 
Felix Zielcke

Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 470 bytes --]

Am Dienstag, den 02.09.2008, 00:46 +0200 schrieb Felix Zielcke:

> I'm too lazy now to make a new patch and go sleeping now.
> [...]
> 
> I hope that Marco could have a quick look over it especially the
> changelog part :)

Final patch attached.
In changelog I had again past and present mixed and I just use now the
`normal' types instead of grub_uintN_t, seems like that's more used on
util/
So if there are no objections I'd like to commit this :)

-- 
Felix Zielcke

[-- Attachment #2: getroot.diff.2 --]
[-- Type: text/plain, Size: 2598 bytes --]

2008-08-14  Felix Zielcke  <fzielcke@z-51.de>

        * util/getroot.c: Include <config.h>.
        (grub_util_get_grub_dev): Rewritten to use asprintf for mdraid devices,
        add support for /dev/md/N devices and handles LVM double dash escaping.

Index: util/getroot.c
===================================================================
--- util/getroot.c	(Revision 1845)
+++ util/getroot.c	(Arbeitskopie)
@@ -17,6 +17,7 @@
  *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <config.h>
 #include <sys/stat.h>
 #include <unistd.h>
 #include <string.h>
@@ -417,58 +418,45 @@ grub_util_get_grub_dev (const char *os_d
   switch (grub_util_get_dev_abstraction (os_dev))
     {
     case GRUB_DEV_ABSTRACTION_LVM:
-      grub_dev = xmalloc (strlen (os_dev) - 12 + 1);
 
-      strcpy (grub_dev, os_dev + 12);
+      {
+	unsigned short i, j;
+	const unsigned char k = sizeof ("/dev/mapper/") - 1;
+	const unsigned short l = strlen (os_dev) - k + 1;
 
-      break;
+	grub_dev = xmalloc (strlen (os_dev) - k + 1);
+	  
+	for (i = 0, j = 0; i < l; i++, j++)
+	  {
+	    grub_dev[j] = os_dev[k + i];
+	    if (os_dev[k + i] == '-' && os_dev[k + i + 1] == '-')
+	      i++;
+	  }
+      }
 
-    case GRUB_DEV_ABSTRACTION_RAID:
-      grub_dev = xmalloc (20);
+    break;
 
+    case GRUB_DEV_ABSTRACTION_RAID:
+      
       if (os_dev[7] == '_' && os_dev[8] == 'd')
 	{
-	  const char *p;
-
 	  /* This a partitionable RAID device of the form /dev/md_dNNpMM. */
-	  int i;
-
-	  grub_dev[0] = 'm';
-	  grub_dev[1] = 'd';
-	  i = 2;
 	  
-	  p = os_dev + 9;
-	  while (*p >= '0' && *p <= '9')
-	    {
-	      grub_dev[i] = *p;
-	      i++;
-	      p++;
-	    }
-
-	  if (*p == '\0')
-	    grub_dev[i] = '\0';
-	  else if (*p == 'p')
-	    {
-	      p++;
-	      grub_dev[i] = ',';
-	      i++;
-
-	      while (*p >= '0' && *p <= '9')
-		{
-		  grub_dev[i] = *p;
-		  i++;
-		  p++;
-		}
+	  char *p;
+	  
+	  p = strchr (os_dev, 'p');
+	  if (p)
+	    *p = ',';
 
-	      grub_dev[i] = '\0';
-	    }
-	  else
-	    grub_util_error ("Unknown kind of RAID device `%s'", os_dev);
+	  asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md_d") - 1);
 	}
       else if (os_dev[7] >= '0' && os_dev[7] <= '9')
 	{
-	  memcpy (grub_dev, os_dev + 5, 7);
-	  grub_dev[7] = '\0';
+	  asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md") - 1);
+	}
+      else if (os_dev[7] == '/' && os_dev[8] >= '0' && os_dev[8] <= '9')
+	{
+	  asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md/") - 1);
 	}
       else
 	grub_util_error ("Unknown kind of RAID device `%s'", os_dev);

Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name

[Vesa Jääskeläinen]

Felix Zielcke wrote:
> Am Dienstag, den 02.09.2008, 00:46 +0200 schrieb Felix Zielcke:
> 
>> I'm too lazy now to make a new patch and go sleeping now.
>> [...]
>>
>> I hope that Marco could have a quick look over it especially the
>> changelog part :)
> 
> Final patch attached.
> In changelog I had again past and present mixed and I just use now the
> `normal' types instead of grub_uintN_t, seems like that's more used on
> util/
> So if there are no objections I'd like to commit this :)

> +	unsigned short i, j;
> +	const unsigned char k = sizeof ("/dev/mapper/") - 1;
> +	const unsigned short l = strlen (os_dev) - k + 1;

sizeof returns type of size_t so it would be good that char k uses that.
I am a bit surprised that this didn't generate compiler warning? And
there is no reason to define integers as constants unless you really
want to make sure they don't change :)

I assume we have grub_size_t or comparable there.

> +	const unsigned short l = strlen (os_dev) - k + 1;
>  
> -      break;
> +	grub_dev = xmalloc (strlen (os_dev) - k + 1);

if you already calculate something to variable it would be nice to
re-use that calculation again.

> +	for (i = 0, j = 0; i < l; i++, j++)
> +	  {
> +	    grub_dev[j] = os_dev[k + i];
> +	    if (os_dev[k + i] == '-' && os_dev[k + i + 1] == '-')
> +	      i++;
> +	  }

Can't you index i: k <= i < strlen(os_dev)? Would make this a bit more
readable. Or as you could just increment k in every loop and use that to
index. Your 0 <= j < l should limit char array.

You could use a bit more descriptive names like l->len, k->start/offset.
For indexes we quite often use i,j,k.

> +	  p = strchr (os_dev, 'p');
> +	  if (p)
> +	    *p = ',';

It is usually a bad idea to modify source string.

Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name

[Felix Zielcke]

[-- Attachment #1: Type: text/plain, Size: 1273 bytes --]

Am Dienstag, den 02.09.2008, 17:59 +0300 schrieb Vesa Jääskeläinen: 
> Felix Zielcke wrote:
> 
> sizeof returns type of size_t so it would be good that char k uses that.
> I am a bit surprised that this didn't generate compiler warning?

Well somewhere hidden in a mail from me, I think I already wrote that I
already got the idea to make these warnings more visible :)
I didn't look yet further into it, maybe even a [RFC] topic should be
made for this.
I'm thinking about how linux kernel compiling works.

Actually I haven't checked that code ;)

> And there is no reason to define integers as constants unless you really
> want to make sure they don't change :)

This is just a thing left from my previous coding experience.
As I started to code I personally just prefer that, but yeah gcc is so
nice and optimizes (almost) everything for us and this code is so little
anyway :)

> > +	  p = strchr (os_dev, 'p');
> > +	  if (p)
> > +	    *p = ',';
> 
> It is usually a bad idea to modify source string.

Urm right this is util/ no need to save a few bytes :)

Thanks for your suggestions Vesa.
It really looks now better compared to the first code I sent about this
problem and it only uses now 3 variables :)

-- 
Felix Zielcke

[-- Attachment #2: getroot.diff.3 --]
[-- Type: text/plain, Size: 2621 bytes --]

2008-09-02  Felix Zielcke  <fzielcke@z-51.de>

        * util/getroot.c: Include <config.h>.
        (grub_util_get_grub_dev): Rewritten to use asprintf for mdraid devices,
        add support for /dev/md/N devices and handle LVM double dash escaping.

Index: util/getroot.c
===================================================================
--- util/getroot.c	(Revision 1845)
+++ util/getroot.c	(Arbeitskopie)
@@ -17,6 +17,7 @@
  *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <config.h>
 #include <sys/stat.h>
 #include <unistd.h>
 #include <string.h>
@@ -417,62 +418,52 @@ grub_util_get_grub_dev (const char *os_d
   switch (grub_util_get_dev_abstraction (os_dev))
     {
     case GRUB_DEV_ABSTRACTION_LVM:
-      grub_dev = xmalloc (strlen (os_dev) - 12 + 1);
-
-      strcpy (grub_dev, os_dev + 12);
 
+      {
+	unsigned short i, len;
+	grub_size_t offset = sizeof ("/dev/mapper/") - 1;
+
+	len = strlen (os_dev) - offset;
+	grub_dev = xmalloc (len);
+
+	for (i = 0; i <= len; i++, offset++)
+	  {
+	    grub_dev[i] = os_dev[offset];
+	    if (os_dev[offset] == '-' && os_dev[offset + 1] == '-')
+	      offset++;
+	  }
+      }
+      
       break;
 
     case GRUB_DEV_ABSTRACTION_RAID:
-      grub_dev = xmalloc (20);
 
       if (os_dev[7] == '_' && os_dev[8] == 'd')
 	{
-	  const char *p;
-
 	  /* This a partitionable RAID device of the form /dev/md_dNNpMM. */
-	  int i;
-
-	  grub_dev[0] = 'm';
-	  grub_dev[1] = 'd';
-	  i = 2;
 	  
-	  p = os_dev + 9;
-	  while (*p >= '0' && *p <= '9')
-	    {
-	      grub_dev[i] = *p;
-	      i++;
-	      p++;
-	    }
+	  char *p , *q;
 
-	  if (*p == '\0')
-	    grub_dev[i] = '\0';
-	  else if (*p == 'p')
-	    {
-	      p++;
-	      grub_dev[i] = ',';
-	      i++;
-
-	      while (*p >= '0' && *p <= '9')
-		{
-		  grub_dev[i] = *p;
-		  i++;
-		  p++;
-		}
+	  p = strdup (os_dev + sizeof ("/dev/md_d") - 1);
 
-	      grub_dev[i] = '\0';
-	    }
-	  else
-	    grub_util_error ("Unknown kind of RAID device `%s'", os_dev);
+	  q = strchr (p, 'p');
+	  if (q)
+	    *q = ',';
+
+	  asprintf (&grub_dev, "md%s", p);
+	  free (p);
 	}
       else if (os_dev[7] >= '0' && os_dev[7] <= '9')
 	{
-	  memcpy (grub_dev, os_dev + 5, 7);
-	  grub_dev[7] = '\0';
+	  asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md") - 1);
+	}
+      else if (os_dev[7] == '/' && os_dev[8] >= '0' && os_dev[8] <= '9')
+	{
+	  asprintf (&grub_dev, "md%s", os_dev + sizeof ("/dev/md/") - 1);
 	}
       else
 	grub_util_error ("Unknown kind of RAID device `%s'", os_dev);
-
+      
       break;
 
     default:  /* GRUB_DEV_ABSTRACTION_NONE */

Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name

[Vesa Jääskeläinen]

Felix Zielcke wrote:
> Am Dienstag, den 02.09.2008, 17:59 +0300 schrieb Vesa Jääskeläinen: 
>> Felix Zielcke wrote:
>>
>> sizeof returns type of size_t so it would be good that char k uses that.
>> I am a bit surprised that this didn't generate compiler warning?
> 
> Well somewhere hidden in a mail from me, I think I already wrote that I
> already got the idea to make these warnings more visible :)
> I didn't look yet further into it, maybe even a [RFC] topic should be
> made for this.

I think Colin had patch already in list archives... That would be
suitable for this. Perhaps you guys could review that and get that in.

Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name

[Felix Zielcke]

Commited in the hope that Marco isn't angry with me ;)

and I noticed I used still past once and there was a reason for len = +1
and if < len not <= len

Am Dienstag, den 02.09.2008, 17:59 +0200 schrieb Felix Zielcke:
> Am Dienstag, den 02.09.2008, 17:59 +0300 schrieb Vesa Jääskeläinen: 
> > Felix Zielcke wrote:
> > 
> > sizeof returns type of size_t so it would be good that char k uses that.
> > I am a bit surprised that this didn't generate compiler warning?
> 
> Well somewhere hidden in a mail from me, I think I already wrote that I
> already got the idea to make these warnings more visible :)
> I didn't look yet further into it, maybe even a [RFC] topic should be
> made for this.
> I'm thinking about how linux kernel compiling works.
> 
> Actually I haven't checked that code ;)
> 
> > And there is no reason to define integers as constants unless you really
> > want to make sure they don't change :)
> 
> This is just a thing left from my previous coding experience.
> As I started to code I personally just prefer that, but yeah gcc is so
> nice and optimizes (almost) everything for us and this code is so little
> anyway :)
> 
> > > +	  p = strchr (os_dev, 'p');
> > > +	  if (p)
> > > +	    *p = ',';
> > 
> > It is usually a bad idea to modify source string.
> 
> Urm right this is util/ no need to save a few bytes :)
> 
> Thanks for your suggestions Vesa.
> It really looks now better compared to the first code I sent about this
> problem and it only uses now 3 variables :)
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
-- 
Felix Zielcke

Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name

[Robert Millan]

On Tue, Sep 02, 2008 at 12:46:11AM +0200, Felix Zielcke wrote:
> 
> I'm now not that sure if that #define k would be okay, seems like Vesa
> isn't liking macros.

If you use macros, please give them meaningful names.  But a macro just for
sizeof("foo") isn't much of an improvemtn, since the resulting code is the
same, and it just makes it less readable.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."