| Message ID | 1399662217-31148-4-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 9 May 2014 21:03:23 +0200 Stefan Hajnoczi <stefanha@redhat.com> wrote: > From: Mike Day <ncmike@ncultra.org> > > The help message for qemu-img lists the supported block formats, of > which there are 27 as of version 2.0.50. The formats are printed in > the order of their driver's position in a linked list, which appears > random. This patch prints the formats in sorted order, making it > easier to read and to find a specific format in the list. > > [Added suggestions from Fam Zheng <famz@redhat.com> to declare variables > at the top of the scope in help() and to omit explicit cast for void* > opaque. > --Stefan] > > Signed-off-by: Mike Day <ncmike@ncultra.org> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > qemu-img.c | 28 +++++++++++++++++++++++++--- > 1 file changed, 25 insertions(+), 3 deletions(-) > > diff --git a/qemu-img.c b/qemu-img.c > index 96f4463..317bc6c 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > +static void add_format_to_seq(void *opaque, const char *fmt_name) > +{ > + GSequence *seq = opaque; > + > + if (!g_sequence_lookup(seq, (gpointer)fmt_name, > + compare_data, NULL)) { > + g_sequence_insert_sorted(seq, (gpointer)fmt_name, > + compare_data, NULL); > + } > } Now that this has hit master, I noticed that this breaks the build on my build server: /home/cohuck/git/qemu/qemu-img.c: In function ‘add_format_to_seq’: /home/cohuck/git/qemu/qemu-img.c:73: warning: implicit declaration of function ‘g_sequence_lookup’ /home/cohuck/git/qemu/qemu-img.c:73: warning: nested extern declaration of ‘g_sequence_lookup’ qemu-img.o: In function `add_format_to_seq': /home/cohuck/git/qemu/qemu-img.c:73: undefined reference to `g_sequence_lookup' collect2: ld returned 1 exit status g_sequence_lookup has been added with glib 2.28, and this box has 2.22.5. configure looks for glib >= 2.12 (2.20 for mingw).
On Tue, May 13, 2014 at 10:18 AM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > qemu-img.o: In function `add_format_to_seq': > /home/cohuck/git/qemu/qemu-img.c:73: undefined reference to `g_sequence_lookup' > collect2: ld returned 1 exit status > > g_sequence_lookup has been added with glib 2.28, and this box has > 2.22.5. configure looks for glib >= 2.12 (2.20 for mingw). g_sequence_lookup is there because I was getting duplicate formats in the format list, as in "vfat vfat vfat qcow." That was a temporary situation. the lookup serves to guarantee each format is unique in the list. It may not be needed. Mike
On Tue, May 13, 2014 at 11:40:47AM -0400, Mike Day wrote: > On Tue, May 13, 2014 at 10:18 AM, Cornelia Huck > <cornelia.huck@de.ibm.com> wrote: > > > > qemu-img.o: In function `add_format_to_seq': > > /home/cohuck/git/qemu/qemu-img.c:73: undefined reference to `g_sequence_lookup' > > collect2: ld returned 1 exit status > > > > g_sequence_lookup has been added with glib 2.28, and this box has > > 2.22.5. configure looks for glib >= 2.12 (2.20 for mingw). > > g_sequence_lookup is there because I was getting duplicate formats in > the format list, as in "vfat vfat vfat qcow." That was a temporary > situation. the lookup serves to guarantee each format is unique in the > list. It may not be needed. Jeff Cody recently wanted to eliminate duplicate entries in the list. I thought part of your intention was to address the duplicates with your patch. We can back out the sequence API if it's not supported on older glib but it would be nice to eliminate duplicates later, too. Stefan
On Wed, May 14, 2014 at 8:35 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > Jeff Cody recently wanted to eliminate duplicate entries in the list. I > thought part of your intention was to address the duplicates with your > patch. > > We can back out the sequence API if it's not supported on older glib but > it would be nice to eliminate duplicates later, too. I agree. I can submit an additional patch that uses an older API. What, exactly is the cause of duplicate entries in the list? I've only seen it one time but its disconcerting when it happens. Mike
On Wed, 05/14 09:27, Mike Day wrote: > On Wed, May 14, 2014 at 8:35 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > > Jeff Cody recently wanted to eliminate duplicate entries in the list. I > > thought part of your intention was to address the duplicates with your > > patch. > > > > We can back out the sequence API if it's not supported on older glib but > > it would be nice to eliminate duplicates later, too. > > I agree. I can submit an additional patch that uses an older API. > What, exactly is the cause of duplicate entries in the list? For example in protocol drivers, multiple BlockDrivers are registered with the same format name, although the protocol names are different: nbd gluster sheepdog In my build I see 3 nbd's. Fam
On Wed, May 14, 2014 at 09:27:31AM -0400, Mike Day wrote: > On Wed, May 14, 2014 at 8:35 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > > Jeff Cody recently wanted to eliminate duplicate entries in the list. I > > thought part of your intention was to address the duplicates with your > > patch. > > > > We can back out the sequence API if it's not supported on older glib but > > it would be nice to eliminate duplicates later, too. > > I agree. I can submit an additional patch that uses an older API. > What, exactly is the cause of duplicate entries in the list? I've only > seen it one time but its disconcerting when it happens. > Some block drivers register multiple drivers for a given format name, for instance, gluster: https://github.com/qemu/qemu/blob/master/block/gluster.c#L818 static void bdrv_gluster_init(void) { bdrv_register(&bdrv_gluster_rdma); bdrv_register(&bdrv_gluster_unix); bdrv_register(&bdrv_gluster_tcp); bdrv_register(&bdrv_gluster); } Each of those drivers has a format_name of "gluster", in this example. In qemu-img, it lists the supported formats by simply calling bdrv_iterate_format(), which calls a callback function for each format_name in the driver list. Prior to a recent commit, this function did not make distinction on duplicates. As of commit e855e4fb7, duplicates are not longer printed in the help message: e855e4fb7: Ignore duplicate or NULL format_name in bdrv_iterate_format): https://github.com/qemu/qemu/commit/e855e4fb7b97f7f605e1f44427b98022e39e6f8f#diff-ea36ba0f79150cc299732696a069caba
On Wed, May 14, 2014 at 09:40:00PM +0800, Fam Zheng wrote: > On Wed, 05/14 09:27, Mike Day wrote: > > On Wed, May 14, 2014 at 8:35 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > > > Jeff Cody recently wanted to eliminate duplicate entries in the list. I > > > thought part of your intention was to address the duplicates with your > > > patch. > > > > > > We can back out the sequence API if it's not supported on older glib but > > > it would be nice to eliminate duplicates later, too. > > > > I agree. I can submit an additional patch that uses an older API. > > What, exactly is the cause of duplicate entries in the list? > > For example in protocol drivers, multiple BlockDrivers are registered with the > same format name, although the protocol names are different: > > nbd > gluster > sheepdog > > In my build I see 3 nbd's. > > Fam Duplicates should no longer be printed on qemu/master - commit e855e4fb7b has been applied, as of April 29th 2014.
On Wed, May 14, 2014 at 9:43 AM, Jeff Cody <jcody@redhat.com> wrote: > Prior to a recent commit, this function did not make distinction on > duplicates. As of commit e855e4fb7, duplicates are not longer printed > in the help message: > > e855e4fb7: Ignore duplicate or NULL format_name in bdrv_iterate_format): > That makes sense, thanks for the background! Mike
diff --git a/qemu-img.c b/qemu-img.c index 96f4463..317bc6c 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -32,6 +32,7 @@ #include "block/block_int.h" #include "block/qapi.h" #include <getopt.h> +#include <glib.h> #define QEMU_IMG_VERSION "qemu-img version " QEMU_VERSION \ ", Copyright (c) 2004-2008 Fabrice Bellard\n" @@ -55,9 +56,25 @@ typedef enum OutputFormat { #define BDRV_O_FLAGS BDRV_O_CACHE_WB #define BDRV_DEFAULT_CACHE "writeback" -static void format_print(void *opaque, const char *name) +static gint compare_data(gconstpointer a, gconstpointer b, gpointer user) { - printf(" %s", name); + return g_strcmp0(a, b); +} + +static void print_format(gpointer data, gpointer user) +{ + printf(" %s", (char *)data); +} + +static void add_format_to_seq(void *opaque, const char *fmt_name) +{ + GSequence *seq = opaque; + + if (!g_sequence_lookup(seq, (gpointer)fmt_name, + compare_data, NULL)) { + g_sequence_insert_sorted(seq, (gpointer)fmt_name, + compare_data, NULL); + } } static void QEMU_NORETURN GCC_FMT_ATTR(1, 2) error_exit(const char *fmt, ...) @@ -142,10 +159,15 @@ static void QEMU_NORETURN help(void) " '-f' first image format\n" " '-F' second image format\n" " '-s' run in Strict mode - fail on different image size or sector allocation\n"; + GSequence *seq; printf("%s\nSupported formats:", help_msg); - bdrv_iterate_format(format_print, NULL); + seq = g_sequence_new(NULL); + bdrv_iterate_format(add_format_to_seq, seq); + g_sequence_foreach(seq, print_format, NULL); printf("\n"); + g_sequence_free(seq); + exit(EXIT_SUCCESS); }