qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v14 04/20] qemu-img: Add --share-rw option to su


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v14 04/20] qemu-img: Add --share-rw option to subcommands
Date: Fri, 21 Apr 2017 10:35:18 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.0

On 04/21/2017 08:25 AM, Kevin Wolf wrote:
> Am 21.04.2017 um 05:55 hat Fam Zheng geschrieben:
>> Similar to share-rw qdev property, this will force the opened images to
>> allow shared write permission of other programs.
>>
>> Signed-off-by: Fam Zheng <address@hidden>
> 
> General observation: We were considering to make share-rw require
> read-only. Some of the commands converted here always open the image
> read-write, so if we go ahead with the restriction, will the option
> become useless in many of the subcommands?
> 

>>  static BlockBackend *img_open_opts(const char *optstr,
>>                                     QemuOpts *opts, int flags, bool 
>> writethrough,
>> -                                   bool quiet)
>> +                                   bool quiet, bool share_rw)
>>  {
>>      QDict *options;
>>      Error *local_err = NULL;
>>      BlockBackend *blk;
>>      options = qemu_opts_to_qdict(opts, NULL);
>> +    if (share_rw) {
>> +        qdict_put(options, BDRV_OPT_FORCE_SHARED_WRITE, 
>> qbool_from_bool(true));
>> +    }
> 
> It's interesting that you chose a conditional qdict_put for true rather
> than an unconditional one for share_rw here. The difference becomes
> visible when someone sets both -U and share-rw=off; we need to decide
> which one should take precedence.
> 
> Generally, we always give explicit options the precedence, so if we were
> to follow suit here, we would set share-rw here only if the option isn't
> already set. For strings, we have qdict_set_default_str() to achieve
> this, for bools we probably need a new function (or does Eric's series
> which introduces qdict_put_bool() also introduce a similar function,
> like some qdict_set_default_bool?)

It doesn't (yet), but I could add one if it is a common pattern that we
envision using.


>> @@ -3126,6 +3182,9 @@ static int img_rebase(int argc, char **argv)
>>              qdict_put(options, "driver", 
>> qstring_from_str(bs->backing_format));
>>          }
>>  
>> +        if (share_rw) {
>> +            qdict_put(options, BDRV_OPT_FORCE_SHARED_WRITE, 
>> qbool_from_bool(true));
> 
> This is longer than 80 lines and wrapping wouldn't make it unreadable. I
> think there are more similar instances in this series (even though you
> replied to the patchew mail that they are intentional).

This one would indeed benefit from my patch for qdict_put_bool().

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]