[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: |
Fam Zheng |
Subject: |
Re: [Qemu-block] [PATCH v14 04/20] qemu-img: Add --share-rw option to subcommands |
Date: |
Mon, 24 Apr 2017 14:10:30 +0800 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
On Fri, 04/21 15:25, 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?
share-rw on qdev doesn't require read-only, so I personally perfer we follow
that manner. Because even with --share-rw for the read-write commands,
the image is still protected from corruption by the fact that the other side
probably uses non-share-rw.
But on the other hand, we can always add the option when necessary, so it's okay
to leave them as is. If you insist, I can remove them in next version.
>
> > qemu-img.c | 155
> > +++++++++++++++++++++++++++++++++++++++++++++++--------------
> > 1 file changed, 119 insertions(+), 36 deletions(-)
> >
> > diff --git a/qemu-img.c b/qemu-img.c
> > index ed24371..df88a79 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -28,6 +28,7 @@
> > #include "qapi/qobject-output-visitor.h"
> > #include "qapi/qmp/qerror.h"
> > #include "qapi/qmp/qjson.h"
> > +#include "qapi/qmp/qbool.h"
> > #include "qemu/cutils.h"
> > #include "qemu/config-file.h"
> > #include "qemu/option.h"
> > @@ -283,12 +284,15 @@ static int img_open_password(BlockBackend *blk, const
> > char *filename,
> >
> > 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.
I don't have a preference here. Setting both -U and share-rw=off is
inconsistent, it's not a problem to yield an "undefined" result.
>
> 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?)
>
> > blk = blk_new_open(NULL, NULL, options, flags, &local_err);
> > if (!blk) {
> > error_reportf_err(local_err, "Could not open '%s': ", optstr);
>
> > @@ -2985,6 +3035,7 @@ static int img_rebase(int argc, char **argv)
> > int c, flags, src_flags, ret;
> > bool writethrough, src_writethrough;
> > int unsafe = 0;
> > + bool share_rw = 0;
>
> Not false?
Will fix.
>
> > int progress = 0;
> > bool quiet = false;
> > Error *local_err = NULL;
> > @@ -3001,9 +3052,10 @@ static int img_rebase(int argc, char **argv)
> > {"help", no_argument, 0, 'h'},
> > {"object", required_argument, 0, OPTION_OBJECT},
> > {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
> > + {"share-rw", no_argument, 0, 'U'},
> > {0, 0, 0, 0}
> > };
> > - c = getopt_long(argc, argv, ":hf:F:b:upt:T:q",
> > + c = getopt_long(argc, argv, ":hf:F:b:upt:T:qU",
> > long_options, NULL);
> > if (c == -1) {
> > break;
> > @@ -3053,6 +3105,9 @@ static int img_rebase(int argc, char **argv)
> > case OPTION_IMAGE_OPTS:
> > image_opts = true;
> > break;
> > + case 'U':
> > + share_rw = true;
> > + break;
> > }
> > }
> >
> > @@ -3101,7 +3156,8 @@ static int img_rebase(int argc, char **argv)
> > * Ignore the old backing file for unsafe rebase in case we want to
> > correct
> > * the reference to a renamed or moved backing file.
> > */
> > - blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet);
> > + blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet,
> > + share_rw);
> > if (!blk) {
> > ret = -1;
> > goto out;
> > @@ -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).
OK, I can wrap it.
>
> > + }
> > bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
> > blk_old_backing = blk_new_open(backing_name, NULL,
> > options, src_flags, &local_err);
>
> Why don't you apply share_rw to blk_new_backing, which is opened a few
> lines down from here?
I missed that one, will fix.
Fam
- [Qemu-block] [PATCH v14 01/20] block: Add, parse and store "force-shared-write" option, (continued)
[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