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 <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?
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