[libvirt] [PATCH] Fix enumeration of partitions in disks with a trailing digit in path
Disks with a trailing digit in their path (eg /dev/loop0 or /dev/dm0) have an extra 'p' appended before the partition number (eg, to form /dev/loop0p1 not /dev/loop01). Fix the partition lookup to append this extra 'p' when required * src/storage/parthelper.c: Add a 'p' before partition number if required --- src/storage/parthelper.c | 15 +++++++++++---- 1 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/storage/parthelper.c b/src/storage/parthelper.c index 5626cd2..28d88c9 100644 --- a/src/storage/parthelper.c +++ b/src/storage/parthelper.c @@ -36,6 +36,8 @@ #include <stdio.h> #include <string.h> +#include "c-ctype.h" + /* we don't need to include the full internal.h just for this */ #define STREQ(a,b) (strcmp(a,b) == 0) @@ -56,6 +58,8 @@ int main(int argc, char **argv) PedDisk *disk; PedPartition *part; int cmd = DISK_LAYOUT; + const char *path; + const char *partsep; if (argc == 3 && STREQ(argv[2], "-g")) { cmd = DISK_GEOMETRY; @@ -64,8 +68,11 @@ int main(int argc, char **argv) return 1; } - if ((dev = ped_device_get(argv[1])) == NULL) { - fprintf(stderr, "unable to access device %s\n", argv[1]); + path = argv[1]; + partsep = c_isdigit(path[strlen(path)-1]) ? "p" : ""; + + if ((dev = ped_device_get(path)) == NULL) { + fprintf(stderr, "unable to access device %s\n", path); return 2; } @@ -117,8 +124,8 @@ int main(int argc, char **argv) * in bytes, not the last sector number */ if (part->num != -1) { - printf("%s%d%c%s%c%s%c%llu%c%llu%c%llu%c", - part->geom.dev->path, + printf("%s%s%d%c%s%c%s%c%llu%c%llu%c%llu%c", + path, partsep, part->num, '\0', type, '\0', content, '\0', -- 1.6.5.2
On 06/08/2010 11:27 AM, Daniel P. Berrange wrote:
Disks with a trailing digit in their path (eg /dev/loop0 or /dev/dm0) have an extra 'p' appended before the partition number (eg, to form /dev/loop0p1 not /dev/loop01). Fix the partition lookup to append this extra 'p' when required
* src/storage/parthelper.c: Add a 'p' before partition number if required
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 06/08/2010 11:27 AM, Daniel P. Berrange wrote:
Disks with a trailing digit in their path (eg /dev/loop0 or /dev/dm0) have an extra 'p' appended before the partition number (eg, to form /dev/loop0p1 not /dev/loop01). Fix the partition lookup to append this extra 'p' when required
* src/storage/parthelper.c: Add a 'p' before partition number if required --- src/storage/parthelper.c | 15 +++++++++++---- 1 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/storage/parthelper.c b/src/storage/parthelper.c index 5626cd2..28d88c9 100644 --- a/src/storage/parthelper.c +++ b/src/storage/parthelper.c @@ -36,6 +36,8 @@ #include <stdio.h> #include <string.h>
+#include "c-ctype.h" + ...
- if ((dev = ped_device_get(argv[1])) == NULL) { - fprintf(stderr, "unable to access device %s\n", argv[1]); + path = argv[1]; + partsep = c_isdigit(path[strlen(path)-1]) ? "p" : "";
If using gcc and -O, this works just fine (c_isdigit is a macro in that case). But if you set CFLAGS=-g, with no optimization (or if you don't use gcc), then this results in a link error: libvirt_parthelper-parthelper.o: In function `main': /home/remote/eblake/libvirt/src/storage/parthelper.c:72: undefined reference to `c_isdigit' I'm working on a followup patch, because it is (sometimes) necessary to link in -lgnulib when building parthelper. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Daniel's patch works with gcc and CFLAGS containing -O (the autoconf default), but fails with non-gcc or with other CFLAGS (such as -g), since c-ctype.h declares c_isdigit as a macro only for certain compilation settings. * src/Makefile.am (libvirt_parthelper_LDFLAGS): Add gnulib library, for when c_isdigit is not a macro. --- I don't see Daniel's patch applied upstream yet, but either this should be squashed in, or applied immediately after. However, given the state of: https://bugzilla.redhat.com/show_bug.cgi?id=593785 I would recommend that this patch remain separate to make it easier for backporting. src/Makefile.am | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 6bdf73c..dac12b9 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1037,7 +1037,7 @@ libexec_PROGRAMS += libvirt_parthelper libvirt_parthelper_SOURCES = $(STORAGE_HELPER_DISK_SOURCES) libvirt_parthelper_LDFLAGS = $(WARN_LDFLAGS) $(COVERAGE_LDFLAGS) -libvirt_parthelper_LDADD = $(LIBPARTED_LIBS) +libvirt_parthelper_LDADD = $(LIBPARTED_LIBS) ../gnulib/lib/libgnu.la libvirt_parthelper_CFLAGS = $(LIBPARTED_CFLAGS) endif endif -- 1.7.0.1
Daniel's patch works with gcc and CFLAGS containing -O (the autoconf default), but fails with non-gcc or with other CFLAGS (such as -g), since c-ctype.h declares c_isdigit as a macro only for certain compilation settings. * src/Makefile.am (libvirt_parthelper_LDFLAGS): Add gnulib library, for when c_isdigit is not a macro. * src/storage/parthelper.c (main): Avoid out-of-bounds dereference, noticed by Jim Meyering. --- change from v1: fix issue noticed by Jim src/Makefile.am | 2 +- src/storage/parthelper.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 6bdf73c..2129960 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1038,7 +1038,7 @@ libexec_PROGRAMS += libvirt_parthelper libvirt_parthelper_SOURCES = $(STORAGE_HELPER_DISK_SOURCES) libvirt_parthelper_LDFLAGS = $(WARN_LDFLAGS) $(COVERAGE_LDFLAGS) libvirt_parthelper_LDADD = $(LIBPARTED_LIBS) -libvirt_parthelper_CFLAGS = $(LIBPARTED_CFLAGS) +libvirt_parthelper_CFLAGS = $(LIBPARTED_CFLAGS) ../gnulib/lib/libgnu.la endif endif EXTRA_DIST += $(STORAGE_HELPER_DISK_SOURCES) diff --git a/src/storage/parthelper.c b/src/storage/parthelper.c index 28d88c9..ca74456 100644 --- a/src/storage/parthelper.c +++ b/src/storage/parthelper.c @@ -69,7 +69,7 @@ int main(int argc, char **argv) } path = argv[1]; - partsep = c_isdigit(path[strlen(path)-1]) ? "p" : ""; + partsep = path && *path && c_isdigit(path[strlen(path)-1]) ? "p" : ""; if ((dev = ped_device_get(path)) == NULL) { fprintf(stderr, "unable to access device %s\n", path); -- 1.7.0.1
Daniel's patch works with gcc and CFLAGS containing -O (the autoconf default), but fails with non-gcc or with other CFLAGS (such as -g), since c-ctype.h declares c_isdigit as a macro only for certain compilation settings. * src/Makefile.am (libvirt_parthelper_LDFLAGS): Add gnulib library, for when c_isdigit is not a macro. * src/storage/parthelper.c (main): Avoid out-of-bounds dereference, noticed by Jim Meyering. --- change in v3: earlier code already guarantees that path is non-null if we got this far, but not whether it is non-empty src/Makefile.am | 2 +- src/storage/parthelper.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 6bdf73c..2129960 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1035,13 +1035,13 @@ if WITH_STORAGE_DISK if WITH_LIBVIRTD libexec_PROGRAMS += libvirt_parthelper libvirt_parthelper_SOURCES = $(STORAGE_HELPER_DISK_SOURCES) libvirt_parthelper_LDFLAGS = $(WARN_LDFLAGS) $(COVERAGE_LDFLAGS) libvirt_parthelper_LDADD = $(LIBPARTED_LIBS) -libvirt_parthelper_CFLAGS = $(LIBPARTED_CFLAGS) +libvirt_parthelper_CFLAGS = $(LIBPARTED_CFLAGS) ../gnulib/lib/libgnu.la endif endif EXTRA_DIST += $(STORAGE_HELPER_DISK_SOURCES) if WITH_LXC diff --git a/src/storage/parthelper.c b/src/storage/parthelper.c index 28d88c9..ca74456 100644 --- a/src/storage/parthelper.c +++ b/src/storage/parthelper.c @@ -66,13 +66,13 @@ int main(int argc, char **argv) } else if (argc != 2) { fprintf(stderr, "syntax: %s DEVICE [-g]\n", argv[0]); return 1; } path = argv[1]; - partsep = c_isdigit(path[strlen(path)-1]) ? "p" : ""; + partsep = *path && c_isdigit(path[strlen(path)-1]) ? "p" : ""; if ((dev = ped_device_get(path)) == NULL) { fprintf(stderr, "unable to access device %s\n", path); return 2; } -- 1.7.0.1
Eric Blake wrote:
Daniel's patch works with gcc and CFLAGS containing -O (the autoconf default), but fails with non-gcc or with other CFLAGS (such as -g), since c-ctype.h declares c_isdigit as a macro only for certain compilation settings.
* src/Makefile.am (libvirt_parthelper_LDFLAGS): Add gnulib library, for when c_isdigit is not a macro. * src/storage/parthelper.c (main): Avoid out-of-bounds dereference, noticed by Jim Meyering. ---
change in v3: earlier code already guarantees that path is non-null if we got this far, but not whether it is non-empty
src/Makefile.am | 2 +- src/storage/parthelper.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am index 6bdf73c..2129960 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1035,13 +1035,13 @@ if WITH_STORAGE_DISK if WITH_LIBVIRTD libexec_PROGRAMS += libvirt_parthelper
libvirt_parthelper_SOURCES = $(STORAGE_HELPER_DISK_SOURCES) libvirt_parthelper_LDFLAGS = $(WARN_LDFLAGS) $(COVERAGE_LDFLAGS) libvirt_parthelper_LDADD = $(LIBPARTED_LIBS) -libvirt_parthelper_CFLAGS = $(LIBPARTED_CFLAGS) +libvirt_parthelper_CFLAGS = $(LIBPARTED_CFLAGS) ../gnulib/lib/libgnu.la endif endif EXTRA_DIST += $(STORAGE_HELPER_DISK_SOURCES)
if WITH_LXC diff --git a/src/storage/parthelper.c b/src/storage/parthelper.c index 28d88c9..ca74456 100644 --- a/src/storage/parthelper.c +++ b/src/storage/parthelper.c @@ -66,13 +66,13 @@ int main(int argc, char **argv) } else if (argc != 2) { fprintf(stderr, "syntax: %s DEVICE [-g]\n", argv[0]); return 1; }
path = argv[1]; - partsep = c_isdigit(path[strlen(path)-1]) ? "p" : ""; + partsep = *path && c_isdigit(path[strlen(path)-1]) ? "p" : "";
if ((dev = ped_device_get(path)) == NULL) { fprintf(stderr, "unable to access device %s\n", path); return 2; }
Looks good. ACK
On 06/14/2010 03:46 PM, Jim Meyering wrote:
Eric Blake wrote:
Daniel's patch works with gcc and CFLAGS containing -O (the autoconf default), but fails with non-gcc or with other CFLAGS (such as -g), since c-ctype.h declares c_isdigit as a macro only for certain compilation settings.
* src/Makefile.am (libvirt_parthelper_LDFLAGS): Add gnulib library, for when c_isdigit is not a macro. * src/storage/parthelper.c (main): Avoid out-of-bounds dereference, noticed by Jim Meyering.
Looks good. ACK
Thanks; I went ahead and pushed both Daniel's and my patch. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Daniel P. Berrange wrote:
Disks with a trailing digit in their path (eg /dev/loop0 or /dev/dm0) have an extra 'p' appended before the partition number (eg, to form /dev/loop0p1 not /dev/loop01). Fix the partition lookup to append this extra 'p' when required
* src/storage/parthelper.c: Add a 'p' before partition number if required --- src/storage/parthelper.c | 15 +++++++++++---- 1 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/storage/parthelper.c b/src/storage/parthelper.c index 5626cd2..28d88c9 100644 --- a/src/storage/parthelper.c +++ b/src/storage/parthelper.c @@ -36,6 +36,8 @@ #include <stdio.h> #include <string.h>
+#include "c-ctype.h" + /* we don't need to include the full internal.h just for this */ #define STREQ(a,b) (strcmp(a,b) == 0)
@@ -56,6 +58,8 @@ int main(int argc, char **argv) PedDisk *disk; PedPartition *part; int cmd = DISK_LAYOUT; + const char *path; + const char *partsep;
if (argc == 3 && STREQ(argv[2], "-g")) { cmd = DISK_GEOMETRY; @@ -64,8 +68,11 @@ int main(int argc, char **argv) return 1; }
- if ((dev = ped_device_get(argv[1])) == NULL) { - fprintf(stderr, "unable to access device %s\n", argv[1]); + path = argv[1]; + partsep = c_isdigit(path[strlen(path)-1]) ? "p" : "";
Oops. You'll want to rearrange things (perhaps by rejecting the empty string early) so that this doesn't reference path[-1] when given an empty argv[1] (i.e., '').
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Jim Meyering