[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v14 04/20] qemu-img: Add --share-rw option to su
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [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
signature.asc
Description: OpenPGP digital signature
[Qemu-block] [PATCH v14 05/20] qemu-img: Update documentation for --share-rw, Fam Zheng, 2017/04/20
[Qemu-block] [PATCH v14 06/20] qemu-io: Add --share-rw option, Fam Zheng, 2017/04/20
[Qemu-block] [PATCH v14 07/20] iotests: 030: Prepare for image locking, Fam Zheng, 2017/04/20