Re: [Qemu-devel] [PATCH 2/2] qemu-img: Allow cache mode specification for amend

[Max Reitz]

On 21.07.2014 17:57, Eric Blake wrote:
> On 07/19/2014 02:35 PM, Max Reitz wrote:
> > qemu-img amend may extensively modify the target image, depending on the
> > options to be amended (e.g. conversion to qcow2 compat level 0.10 from
> > 1.1 for an image with many unallocated zero clusters). Therefore it
> > makes sense to allow the user to specify the cache mode to be used.
> Extensive modifications implies long-running operation - the 'amend'
> subcommand is a good candidate for the -p progress meter option.  But
> that would be a separate patch.

Yes, this is on my to do list anyway.

> > Signed-off-by: Max Reitz <address@hidden>
> > ---
> >   qemu-img-cmds.hx |  4 ++--
> >   qemu-img.c       | 19 +++++++++++++++----
> >   qemu-img.texi    |  2 +-
> >   3 files changed, 18 insertions(+), 7 deletions(-)
> >
> >   
> > +    cache = BDRV_DEFAULT_CACHE;
> >       for (;;) {
> > -        c = getopt(argc, argv, "hqf:o:");
> > +        c = getopt(argc, argv, "hqf:t:o:");
> >           if (c == -1) {
> >               break;
> >           }
> > @@ -2805,6 +2807,9 @@ static int img_amend(int argc, char **argv)
> >               case 'f':
> >                   fmt = optarg;
> >                   break;
> > +            case 't':
> > +                cache = optarg;
> > +                break;
> >               case 'q':
>
> Pre-existing, so I won't hold up review, but I'm a big fan of having the
> switch block in the same order as the getopt string (that is, we listed
> 'q' before 'f' in the string above, so the cases are out-of-order with
> respect to that string).  The fix can go either way (reshuffle the case
> statements, or reorder the optstring above).  [and for the truly OCD, I
> prefer the optstring in case-insensitive alphabetical order "f:ho:qt:",
> because then it's easier to scan the string to see what letters are
> still available for new options - but that's asking a bit much]

Right, in fact I thought of you when I touched this block; since it was 
already completely out of order, I decided for the minimal change, 
however. I'll redo it in v2.

I personally somehow like to have common switches like -q at the end, I 
don't know why. For now, I'll just reshuffle the getopt() string.

> Reviewed-by: Eric Blake <address@hidden>

Thanks,

Max