Re: [Qemu-devel] [PATCH for-2.7 v2 09/17] qemu-img: Add "-L" option to sub commands

["Denis V. Lunev" <den@virtuozzo.com>]

On 04/16/2016 05:29 PM, Denis V. Lunev wrote:
> On 04/15/2016 06:27 AM, Fam Zheng wrote:
>> If specified, BDRV_O_NO_LOCK flag will be set when opening the image.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>>   qemu-img.c | 89 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++------------
>>   1 file changed, 72 insertions(+), 17 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 1697762..327be44 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
> pls fix help message near
>
> static void QEMU_NORETURN help(void)
> {
>     const char *help_msg =
>            QEMU_IMG_VERSION
>            "usage: qemu-img command [command options]\n"
>            "QEMU disk image utility\n"
>            "\n"
>            "Command syntax:\n"
> #define DEF(option, callback, arg_string)        \
>            "  " arg_string "\n"
> #include "qemu-img-cmds.h"
> #undef DEF
> #undef GEN_DOCS
>
ah, I see this in the next patch. Though the question about img_create
is still actual.



>
> IMHO img_create should also take lock if the image exists already
> to validate that there is no process on top of it.
>
>
>> @@ -600,6 +600,7 @@ static int img_check(int argc, char **argv)
>>       bool quiet = false;
>>       Error *local_err = NULL;
>>       bool image_opts = false;
>> +    bool nolock = false;
>>         fmt = NULL;
>>       output = NULL;
>> @@ -616,7 +617,7 @@ static int img_check(int argc, char **argv)
>>               {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
>>               {0, 0, 0, 0}
>>           };
>> -        c = getopt_long(argc, argv, "hf:r:T:q",
>> +        c = getopt_long(argc, argv, "hf:r:T:qL",
>>                           long_options, &option_index);
>>           if (c == -1) {
>>               break;
>> @@ -650,6 +651,9 @@ static int img_check(int argc, char **argv)
>>           case 'q':
>>               quiet = true;
>>               break;
>> +        case 'L':
>> +            nolock = true;
>> +            break;
> I think that you could fix flags just here as done for 'r', i.e.
>                              flags |= BDRV_O_NO_LOCK
>
> It would be better to switch all other places to this style. Some
> tweaks to old code would be necessary.
>
> Though this is personal and does not block the review.