[Qemu-devel] [PATCH] qemu-img: sort block formats in help message

[Qemu-devel] [PATCH] qemu-img: sort block formats in help message

[Mike Day]

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]

[Removed call to g_sequence_lookup because it breaks the build on
 machines with glib < 2.28.
--Mike]

Signed-off-by: Mike Day <ncmike@ncultra.org>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qemu-img.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 96f4463..93e51d1 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,22 @@ 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;
+
+    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 +156,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);
 }
 
-- 
1.9.0

Re: [Qemu-devel] [PATCH] qemu-img: sort block formats in help message

[Markus Armbruster]

Mike Day <ncmike@ncultra.org> writes:

> 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.

Patch revision notes like these

> [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]
>
> [Removed call to g_sequence_lookup because it breaks the build on
>  machines with glib < 2.28.
> --Mike]
>
> Signed-off-by: Mike Day <ncmike@ncultra.org>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---

... go here, below the '---'.

Furthermore, your previous patch went in already.  We need an
incremental patch to get rid of the dependency on non-ancient glib.

Re: [Qemu-devel] [PATCH] qemu-img: sort block formats in help message

[Kevin Wolf]

Am 13.05.2014 um 18:07 hat Mike Day geschrieben:
> 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]
> 
> [Removed call to g_sequence_lookup because it breaks the build on
>  machines with glib < 2.28.
> --Mike]
> 
> Signed-off-by: Mike Day <ncmike@ncultra.org>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

What happened here? This patch seems to be already applied, and you
included Stefan's SoB as well.

Kevin

Re: [Qemu-devel] [PATCH] qemu-img: sort block formats in help message

[Fam Zheng]

On Wed, 05/14 09:50, Kevin Wolf wrote:
> Am 13.05.2014 um 18:07 hat Mike Day geschrieben:
> > 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]
> > 
> > [Removed call to g_sequence_lookup because it breaks the build on
> >  machines with glib < 2.28.
> > --Mike]
> > 
> > Signed-off-by: Mike Day <ncmike@ncultra.org>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> What happened here? This patch seems to be already applied, and you
> included Stefan's SoB as well.
> 

Mike, please fix on top of current master, instead of squashing the removing of
g_sequence_lookup into your already merged patch.

Fam

Re: [Qemu-devel] [PATCH] qemu-img: sort block formats in help message

[Stefan Hajnoczi]

On Wed, May 14, 2014 at 04:09:14PM +0800, Fam Zheng wrote:
> On Wed, 05/14 09:50, Kevin Wolf wrote:
> > Am 13.05.2014 um 18:07 hat Mike Day geschrieben:
> > > 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]
> > > 
> > > [Removed call to g_sequence_lookup because it breaks the build on
> > >  machines with glib < 2.28.
> > > --Mike]
> > > 
> > > Signed-off-by: Mike Day <ncmike@ncultra.org>
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> > What happened here? This patch seems to be already applied, and you
> > included Stefan's SoB as well.
> > 
> 
> Mike, please fix on top of current master, instead of squashing the removing of
> g_sequence_lookup into your already merged patch.

Yes, we cannot change the git commit history once a commit is in the
public qemu.git repository.  The only options are to:

1. Add a patch on top that fixes the issue.
2. Use git-revert(1) to undo your commit entirely (this adds a new patch
   on top the applies the reverse diff).

In this case #1 seems like a good choice.

Stefan

Re: [Qemu-devel] [PATCH] qemu-img: sort block formats in help message

[Mike Day]

On Wed, May 14, 2014 at 9:02 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> Yes, we cannot change the git commit history once a commit is in the
> public qemu.git repository.  The only options are to:
>
> 1. Add a patch on top that fixes the issue.
> 2. Use git-revert(1) to undo your commit entirely (this adds a new patch
>    on top the applies the reverse diff).
>
> In this case #1 seems like a good choice.

I followed your advice and went with #1, I think its already been pulled.

Mike

[Qemu-devel] [PATCH] qemu-img: sort block formats in help message

[Mike Day]

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.

Signed-off-by: Mike Day <ncmike@ncultra.org>
---
 qemu-img.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

 diff --git a/qemu-img.c b/qemu-img.c
 index 96f4463..d8b7ef4 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"
 @@ -60,6 +61,32 @@ static void format_print(void *opaque, const char *name)
 printf(" %s", name);
 }
 
 +static gint compare_data(gconstpointer a, gconstpointer b, gpointer user)
 +{
 +    return g_strcmp0((const char *)a, (const char *)b);
 +}
 +
 +static void GFunc_print_format(gpointer data, gpointer user)
 +{
 +    format_print(user, data);
 +}
 +
 +static GSequence *init_sequence(void)
 +{
 +    return g_sequence_new(NULL);
 +}
 +
 +static void add_format_to_seq(void *opaque, const char *fmt_name)
 +{
 +    GSequence *seq = (GSequence *)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, ...)
 {
 va_list ap;
 @@ -144,8 +171,12 @@ static void QEMU_NORETURN help(void)
 "  '-s' run in Strict mode - fail on different image size or sector allocation\n";
 
 printf("%s\nSupported formats:", help_msg);
 -    bdrv_iterate_format(format_print, NULL);
 +    GSequence *seq = init_sequence();
 +    bdrv_iterate_format(add_format_to_seq, seq);
 +    g_sequence_foreach(seq, GFunc_print_format, NULL);
 printf("\n");
 +    g_sequence_free(seq);
 +
 exit(EXIT_SUCCESS);
 }
 
 -- 
 1.9.0

Re: [Qemu-devel] [PATCH] qemu-img: sort block formats in help message

[Stefan Hajnoczi]

On Fri, May 02, 2014 at 09:11:15AM -0400, Mike Day wrote:
> 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.

Looks useful, thanks!

> Signed-off-by: Mike Day <ncmike@ncultra.org>
> ---

This patch is whitespace-damaged so git-am(1) refuses to apply it.
Please use git-send-email(1) to send patches so that the right format is
used.

$ git am
Applying: qemu-img: sort block formats in help message
fatal: unrecognized input

>  qemu-img.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
>  diff --git a/qemu-img.c b/qemu-img.c
>  index 96f4463..d8b7ef4 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"
>  @@ -60,6 +61,32 @@ static void format_print(void *opaque, const char *name)
>  printf(" %s", name);
>  }
>  
>  +static gint compare_data(gconstpointer a, gconstpointer b, gpointer user)
>  +{
>  +    return g_strcmp0((const char *)a, (const char *)b);
>  +}
>  +
>  +static void GFunc_print_format(gpointer data, gpointer user)

QEMU coding style is lowercase function and variable names.  The
scripts/checkpatch.pl script should identify coding style violations,
please run it.

>  +{
>  +    format_print(user, data);
>  +}

format_print() isn't called anywhere else, please inline it here since
we no longer need it as its own function.

>  +
>  +static GSequence *init_sequence(void)
>  +{
>  +    return g_sequence_new(NULL);
>  +}

Any reason to hide g_sequence_new(NULL) in it's own helper function?

>  +
>  +static void add_format_to_seq(void *opaque, const char *fmt_name)
>  +{
>  +    GSequence *seq = (GSequence *)opaque;
>  +
>  +    if (!g_sequence_lookup(seq, (gpointer)fmt_name,
>  +                          compare_data, NULL)) {
>  +        g_sequence_insert_sorted(seq, (gpointer)fmt_name,
>  +                                 compare_data, NULL);
>  +    }

The type casts in this patch aren't necessary.  In C void* casts to and
from any type without an explicit cast.  Only C++ demands explicit
casts of void*.

Re: [Qemu-devel] [PATCH] qemu-img: sort block formats in help message

[Mike Day]

Thanks for the review! I've been able to shorten the next version quite a bit.

On Mon, May 5, 2014 at 9:42 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>>  +static void GFunc_print_format(gpointer data, gpointer user)
>
> QEMU coding style is lowercase function and variable names.  The
> scripts/checkpatch.pl script should identify coding style violations,
> please run it.

Yeah this is ugly. I've long had checkpatch.pl configured as my
pre-commit hook. I've confirmed again this morning that these ugly
caps pass through with no error or warning. I'll look at the script.

>>  +
>>  +static void add_format_to_seq(void *opaque, const char *fmt_name)
>>  +{
>>  +    GSequence *seq = (GSequence *)opaque;
>>  +
>>  +    if (!g_sequence_lookup(seq, (gpointer)fmt_name,
>>  +                          compare_data, NULL)) {
>>  +        g_sequence_insert_sorted(seq, (gpointer)fmt_name,
>>  +                                 compare_data, NULL);
>>  +    }
>
> The type casts in this patch aren't necessary.  In C void* casts to and
> from any type without an explicit cast.  Only C++ demands explicit
> casts of void*.

The casts are ugly, and I don't know how to get rid of them here
beyond a pragma.  Due to the block and glib APIs I have to cast away a
const in the second parameter. I'm bracketed on both ends:
g_sequence_* needs that second parameter to be void * (pointer), while
the the block api (bdrv_iterate_format) defines the this function
pointer type as (*)(void *, const char *). Ideas?

Mike