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 <f...@redhat.com>
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