* Re: Bug#494383: grub-pc: Grub2 cannot find LVM volume groups with a dash (-) in the name [not found] <20080808202337.3440.57502.reportbug@sylvester.jejik.com.jejik.com> @ 2008-08-09 10:26 ` Felix Zielcke 2008-08-09 12:55 ` Felix Zielcke 2008-08-10 13:19 ` Sander Marechal 0 siblings, 2 replies; 21+ messages in thread From: Felix Zielcke @ 2008-08-09 10:26 UTC (permalink / raw) To: The development of GRUB 2; +Cc: root, 494383 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: Bug#494383: grub-pc: Grub2 cannot find LVM volume groups with a dash (-) in the name 2008-08-09 10:26 ` Bug#494383: grub-pc: Grub2 cannot find LVM volume groups with a dash (-) in the name Felix Zielcke @ 2008-08-09 12:55 ` Felix Zielcke 2008-08-10 13:19 ` Sander Marechal 1 sibling, 0 replies; 21+ messages in thread From: Felix Zielcke @ 2008-08-09 12:55 UTC (permalink / raw) To: The development of GRUB 2; +Cc: root, 494383 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 ;)) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: Bug#494383: grub-pc: Grub2 cannot find LVM volume groups with a dash (-) in the name 2008-08-09 10:26 ` Bug#494383: grub-pc: Grub2 cannot find LVM volume groups with a dash (-) in the name Felix Zielcke 2008-08-09 12:55 ` Felix Zielcke @ 2008-08-10 13:19 ` Sander Marechal 2008-08-11 16:18 ` [PATCH] " Felix Zielcke 1 sibling, 1 reply; 21+ messages in thread From: Sander Marechal @ 2008-08-10 13:19 UTC (permalink / raw) To: Felix Zielcke; +Cc: The development of GRUB 2, 494383 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name 2008-08-10 13:19 ` Sander Marechal @ 2008-08-11 16:18 ` Felix Zielcke 2008-08-11 21:48 ` Sander Marechal 2008-08-14 17:34 ` Felix Zielcke 0 siblings, 2 replies; 21+ messages in thread From: Felix Zielcke @ 2008-08-11 16:18 UTC (permalink / raw) To: The development of GRUB 2 [-- 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; ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name 2008-08-11 16:18 ` [PATCH] " Felix Zielcke @ 2008-08-11 21:48 ` Sander Marechal 2008-08-14 17:34 ` Felix Zielcke 1 sibling, 0 replies; 21+ messages in thread From: Sander Marechal @ 2008-08-11 21:48 UTC (permalink / raw) To: The development of GRUB 2 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name 2008-08-11 16:18 ` [PATCH] " Felix Zielcke 2008-08-11 21:48 ` Sander Marechal @ 2008-08-14 17:34 ` Felix Zielcke 2008-08-14 18:03 ` Robert Millan 1 sibling, 1 reply; 21+ messages in thread From: Felix Zielcke @ 2008-08-14 17:34 UTC (permalink / raw) To: The development of GRUB 2 [-- 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; ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name 2008-08-14 17:34 ` Felix Zielcke @ 2008-08-14 18:03 ` Robert Millan 2008-08-14 19:32 ` Felix Zielcke 0 siblings, 1 reply; 21+ messages in thread From: Robert Millan @ 2008-08-14 18:03 UTC (permalink / raw) To: The development of GRUB 2 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." ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name 2008-08-14 18:03 ` Robert Millan @ 2008-08-14 19:32 ` Felix Zielcke 2008-08-14 20:38 ` Robert Millan 0 siblings, 1 reply; 21+ messages in thread From: Felix Zielcke @ 2008-08-14 19:32 UTC (permalink / raw) To: The development of GRUB 2 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 :) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name 2008-08-14 19:32 ` Felix Zielcke @ 2008-08-14 20:38 ` Robert Millan 2008-08-14 21:15 ` Felix Zielcke 0 siblings, 1 reply; 21+ messages in thread From: Robert Millan @ 2008-08-14 20:38 UTC (permalink / raw) To: The development of GRUB 2 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." ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name 2008-08-14 20:38 ` Robert Millan @ 2008-08-14 21:15 ` Felix Zielcke 2008-08-18 15:05 ` Felix Zielcke 0 siblings, 1 reply; 21+ messages in thread From: Felix Zielcke @ 2008-08-14 21:15 UTC (permalink / raw) To: The development of GRUB 2 [-- 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); ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name 2008-08-14 21:15 ` Felix Zielcke @ 2008-08-18 15:05 ` Felix Zielcke 2008-08-18 15:16 ` Felix Zielcke 2008-09-01 22:14 ` Robert Millan 0 siblings, 2 replies; 21+ messages in thread From: Felix Zielcke @ 2008-08-18 15:05 UTC (permalink / raw) To: The development of GRUB 2 [-- 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); ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name 2008-08-18 15:05 ` Felix Zielcke @ 2008-08-18 15:16 ` Felix Zielcke 2008-09-01 7:08 ` Felix Zielcke 2008-09-01 22:14 ` Robert Millan 1 sibling, 1 reply; 21+ messages in thread From: Felix Zielcke @ 2008-08-18 15:16 UTC (permalink / raw) To: The development of GRUB 2 [-- 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); ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name 2008-08-18 15:16 ` Felix Zielcke @ 2008-09-01 7:08 ` Felix Zielcke 0 siblings, 0 replies; 21+ messages in thread From: Felix Zielcke @ 2008-09-01 7:08 UTC (permalink / raw) To: The development of GRUB 2 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name 2008-08-18 15:05 ` Felix Zielcke 2008-08-18 15:16 ` Felix Zielcke @ 2008-09-01 22:14 ` Robert Millan 2008-09-01 22:46 ` Felix Zielcke 1 sibling, 1 reply; 21+ messages in thread From: Robert Millan @ 2008-09-01 22:14 UTC (permalink / raw) To: The development of GRUB 2 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." ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name 2008-09-01 22:14 ` Robert Millan @ 2008-09-01 22:46 ` Felix Zielcke 2008-09-02 8:28 ` Felix Zielcke 2008-09-02 13:37 ` Robert Millan 0 siblings, 2 replies; 21+ messages in thread From: Felix Zielcke @ 2008-09-01 22:46 UTC (permalink / raw) To: The development of GRUB 2 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name 2008-09-01 22:46 ` Felix Zielcke @ 2008-09-02 8:28 ` Felix Zielcke 2008-09-02 14:59 ` Vesa Jääskeläinen 2008-09-02 13:37 ` Robert Millan 1 sibling, 1 reply; 21+ messages in thread From: Felix Zielcke @ 2008-09-02 8:28 UTC (permalink / raw) To: The development of GRUB 2 [-- 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); ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name 2008-09-02 8:28 ` Felix Zielcke @ 2008-09-02 14:59 ` Vesa Jääskeläinen 2008-09-02 15:59 ` Felix Zielcke 0 siblings, 1 reply; 21+ messages in thread From: Vesa Jääskeläinen @ 2008-09-02 14:59 UTC (permalink / raw) To: The development of GRUB 2 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name 2008-09-02 14:59 ` Vesa Jääskeläinen @ 2008-09-02 15:59 ` Felix Zielcke 2008-09-02 16:21 ` Vesa Jääskeläinen 2008-09-04 19:56 ` Felix Zielcke 0 siblings, 2 replies; 21+ messages in thread From: Felix Zielcke @ 2008-09-02 15:59 UTC (permalink / raw) To: The development of GRUB 2 [-- 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 */ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name 2008-09-02 15:59 ` Felix Zielcke @ 2008-09-02 16:21 ` Vesa Jääskeläinen 2008-09-04 19:56 ` Felix Zielcke 1 sibling, 0 replies; 21+ messages in thread From: Vesa Jääskeläinen @ 2008-09-02 16:21 UTC (permalink / raw) To: The development of GRUB 2 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name 2008-09-02 15:59 ` Felix Zielcke 2008-09-02 16:21 ` Vesa Jääskeläinen @ 2008-09-04 19:56 ` Felix Zielcke 1 sibling, 0 replies; 21+ messages in thread From: Felix Zielcke @ 2008-09-04 19:56 UTC (permalink / raw) To: The development of GRUB 2 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] Grub2 cannot find LVM volume groups with a dash (-) in the name 2008-09-01 22:46 ` Felix Zielcke 2008-09-02 8:28 ` Felix Zielcke @ 2008-09-02 13:37 ` Robert Millan 1 sibling, 0 replies; 21+ messages in thread From: Robert Millan @ 2008-09-02 13:37 UTC (permalink / raw) To: The development of GRUB 2 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." ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2008-09-04 19:57 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20080808202337.3440.57502.reportbug@sylvester.jejik.com.jejik.com>
2008-08-09 10:26 ` Bug#494383: grub-pc: Grub2 cannot find LVM volume groups with a dash (-) in the name Felix Zielcke
2008-08-09 12:55 ` Felix Zielcke
2008-08-10 13:19 ` Sander Marechal
2008-08-11 16:18 ` [PATCH] " Felix Zielcke
2008-08-11 21:48 ` Sander Marechal
2008-08-14 17:34 ` Felix Zielcke
2008-08-14 18:03 ` Robert Millan
2008-08-14 19:32 ` Felix Zielcke
2008-08-14 20:38 ` Robert Millan
2008-08-14 21:15 ` Felix Zielcke
2008-08-18 15:05 ` Felix Zielcke
2008-08-18 15:16 ` Felix Zielcke
2008-09-01 7:08 ` Felix Zielcke
2008-09-01 22:14 ` Robert Millan
2008-09-01 22:46 ` Felix Zielcke
2008-09-02 8:28 ` Felix Zielcke
2008-09-02 14:59 ` Vesa Jääskeläinen
2008-09-02 15:59 ` Felix Zielcke
2008-09-02 16:21 ` Vesa Jääskeläinen
2008-09-04 19:56 ` Felix Zielcke
2008-09-02 13:37 ` Robert Millan
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.