[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: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] [PATCH v14 04/20] qemu-img: Add --share-rw option to subcommands |
Date: |
Fri, 21 Apr 2017 15:25:09 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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?
> 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.
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?
> 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).
> + }
> 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?
Kevin
- [Qemu-block] [PATCH v14 00/20] block: Image locking series, Fam Zheng, 2017/04/20
- [Qemu-block] [PATCH v14 01/20] block: Add, parse and store "force-shared-write" option, Fam Zheng, 2017/04/20
- [Qemu-block] [PATCH v14 02/20] qapi: Add 'force-shared-write' to blockdev-add arguments, Fam Zheng, 2017/04/20
- [Qemu-block] [PATCH v14 03/20] block: Respect "force-shared-write" in perm propagating, Fam Zheng, 2017/04/20
- [Qemu-block] [PATCH v14 04/20] qemu-img: Add --share-rw option to subcommands, Fam Zheng, 2017/04/20
- Re: [Qemu-block] [PATCH v14 04/20] qemu-img: Add --share-rw option to subcommands,
Kevin Wolf <=
[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