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

[Kevin Wolf]

Am 24.04.2017 um 08:10 hat Fam Zheng geschrieben:
> 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.

It's not really completely comparable to qdev's share-rw because qdev
only shares writes on the top level (and the qcow2 driver restricts this
again down the tree), while this option propagates all the way down.
Which is why you called the block layer option "force-shared-write".
Maybe that would be the better name here as well.

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

If the other side "probably" uses non-share-rw, then the image is only
"probably" protected. I think you're right about the common case, but if
two qemu instances use force-shared-write=on, then we get actual image
corruption.

As far as I know, our real use cases for the option are read-only: We
want to inspect images which are in use by a VM. Do we have any use
cases for read-write access?

Note that this is different from qdev in that share-rw on the qdev level
affects only the user data and can work e.g. if the guest uses a cluster
file system. But this option affects metadata as well and qcow2 never
supports this, so opening two images read-write at the same time is
guaranteed to corrupt the image.

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

Yes, I think we really need a check in bdrv_open_common() that forbids
force-shared-write=on on writable images. And then, the options in
qemu-img become useless when applied to writable images because they
would only produce errors.

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

We could just check whether the entry already exists and error out.
Maybe that's the best option.

Kevin