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

[Max Reitz <mreitz@redhat.com>]

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 <mreitz@redhat.com>
>> ---
>>   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 <eblake@redhat.com>

Thanks,

Max