Re: [Qemu-devel] [PATCH v14 04/20] qemu-img: Add --share-rw option to subcommands

[Kevin Wolf <kwolf@redhat.com>]

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 <famz@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