Re: [Qemu-devel] [PATCH 06/10] qemu-img: Prepare for locked images

[Kevin Wolf]

Am 22.12.2015 um 22:06 hat Eric Blake geschrieben:
> On 12/22/2015 09:46 AM, Kevin Wolf wrote:
> > This patch extends qemu-img for working with locked images. It prints a
> > helpful error message when trying to access a locked image read-write,
> > and adds a 'qemu-img force-unlock' command as well as a 'qemu-img check
> > -r all --force' option in order to override a lock left behind after a
> > qemu crash.
> > 
> > Signed-off-by: Kevin Wolf <address@hidden>
> > ---
> >  include/block/block.h |  1 +
> >  include/qapi/error.h  |  1 +
> >  qapi/common.json      |  3 +-
> >  qemu-img-cmds.hx      | 10 ++++--
> >  qemu-img.c            | 96 
> > +++++++++++++++++++++++++++++++++++++++++++--------
> >  qemu-img.texi         | 20 ++++++++++-
> >  6 files changed, 113 insertions(+), 18 deletions(-)
> > 
> 
> > +++ b/include/qapi/error.h
> > @@ -102,6 +102,7 @@ typedef enum ErrorClass {
> >      ERROR_CLASS_DEVICE_NOT_ACTIVE = QAPI_ERROR_CLASS_DEVICENOTACTIVE,
> >      ERROR_CLASS_DEVICE_NOT_FOUND = QAPI_ERROR_CLASS_DEVICENOTFOUND,
> >      ERROR_CLASS_KVM_MISSING_CAP = QAPI_ERROR_CLASS_KVMMISSINGCAP,
> > +    ERROR_CLASS_IMAGE_FILE_LOCKED = QAPI_ERROR_CLASS_IMAGEFILELOCKED,
> >  } ErrorClass;
> 
> Wow - a new ErrorClass.  It's been a while since we could justify one of
> these, but I think you might have found a case.

Yes, I think libvirt will need it as well.

That's the whole reason why I consider your input so important for this
series. If a crashed qemu can leave locked images around, libvirt needs
to know how to deal with it. I don't think it's hard to implement at
least something basic, but ideally it should be there before the qemu
release that introduces the locks.

> >  /*
> > diff --git a/qapi/common.json b/qapi/common.json
> > index 9353a7b..1bf6e46 100644
> > --- a/qapi/common.json
> > +++ b/qapi/common.json
> > @@ -27,7 +27,8 @@
> >  { 'enum': 'QapiErrorClass',
> >    # Keep this in sync with ErrorClass in error.h
> >    'data': [ 'GenericError', 'CommandNotFound', 'DeviceEncrypted',
> > -            'DeviceNotActive', 'DeviceNotFound', 'KVMMissingCap' ] }
> > +            'DeviceNotActive', 'DeviceNotFound', 'KVMMissingCap',
> > +            'ImageFileLocked' ] }
> 
> Missing documentation of the new value; should be something like:
> 
> # @ImageFileLocked: the requested operation attempted to write to an
> #    image locked for writing by another process (since 2.6)

Right, thanks for catching that.

> >  
> > +DEF("force-unlock", img_force_unlock,
> > +    "force_unlock [-f fmt] filename")
> 
> So is it force-unlock or force_unlock?  It's our first two-word command
> on the qemu-img CLI, but I strongly prefer '-' (hitting the shift key
> mid-word is a bother for CLI usage).

Just a typo (the underscore is only in the help message).

> > +++ b/qemu-img.c
> > @@ -47,6 +47,7 @@ typedef struct img_cmd_t {
> >  enum {
> >      OPTION_OUTPUT = 256,
> >      OPTION_BACKING_CHAIN = 257,
> > +    OPTION_FORCE = 258,
> >  };
> 
> May conflict with Daniel's proposed patches; I'm sure you two can sort
> out the problems.
> 
> > @@ -206,12 +207,34 @@ static BlockBackend *img_open(const char *id, const 
> > char *filename,
> >      Error *local_err = NULL;
> >      QDict *options = NULL;
> >  
> > +    options = qdict_new();
> >      if (fmt) {
> > -        options = qdict_new();
> >          qdict_put(options, "driver", qstring_from_str(fmt));
> >      }
> > +    QINCREF(options);
> >  
> >      blk = blk_new_open(id, filename, NULL, options, flags, &local_err);
> > +    if (!blk && error_get_class(local_err) == 
> > ERROR_CLASS_IMAGE_FILE_LOCKED) {
> > +        if (force) {
> > +            qdict_put(options, BDRV_OPT_OVERRIDE_LOCK, 
> > qstring_from_str("on"));
> 
> I guess it's safer to try without override and then re-issue with it,
> only when needed, rather than treating 'force' as blindly turning on
> override even when it is not needed to avoid the need for reissuing
> commands.  And probably not observable to the user which of the two
> approaches you use (same end results).

The reason here is that only qcow2 supports locks (and therefore the
lock override). Any other image format would result in an "unknown
option" error.

> > +            blk = blk_new_open(id, filename, NULL, options, flags, NULL);
> 
> Can't the second attempt still fail, for some other reason?  I think
> passing NULL for errp is risky here.  I guess you're saved by the fact
> that blk_new_open() should always return NULL if an error would have
> been set, and that you want to favor reporting the original failure
> (with the class ERROR_CLASS_IMAGE_FILE_LOCKED) rather than the
> second-attempt failure.

I think this might be from before I introduced the error class, and I
wanted to return the original error instead of a potential "unknown
option" error that is introduced here. But I think we can assume that
any driver that returns ERROR_CLASS_IMAGE_FILE_LOCKED also supports the
image override option.

So it seems to me that I should now favour the later error.

> > +            if (blk) {
> > +                error_free(local_err);
> > +            }
> > +        } else {
> > +            error_report("The image file '%s' is locked and cannot be "
> > +                         "opened for write access as this may cause image "
> > +                         "corruption.", filename);
> 
> This completely discards the information in local_err.  Of course, I
> don't know what information you are proposing to store for the actual
> advisory lock extension header.  But let's suppose it were to include
> hostname+pid information on who claimed the lock, rather than just a
> single lock bit.  That additional information in local_err may well be
> worth reporting here rather than just discarding it all.

Hm. Yes. I did it this way to avoid the override-lock=on message, which
isn't helpful in a qemu-img context. Maybe I can somehow drop/replace
the hint of the errp and then use that?

> > +            error_report("If it is locked in error (e.g. because "
> > +                         "of an unclean shutdown) and you are sure that no 
> > "
> > +                         "other processes are working on the image file, 
> > you "
> > +                         "can use 'qemu-img force-unlock' or the --force 
> > flag "
> > +                         "for 'qemu-img check' in order to override this "
> > +                         "check.");
> 
> Long line; I don't know if we want to insert intermediate line breaks.
> Markus may have more opinions on what this should look like.

I don't think we assume any specific terminal size and line breaks in
the wrong place are probably worse than no line breaks at all.

> > +static int img_force_unlock(int argc, char **argv)
> > +{
> > +    BlockBackend *blk;
> > +    const char *format = NULL;
> > +    const char *filename;
> > +    char c;
> > +
> > +    for (;;) {
> > +        c = getopt(argc, argv, "hf:");
> > +        if (c == -1) {
> > +            break;
> > +        }
> > +        switch (c) {
> > +        case '?':
> > +        case 'h':
> > +            help();
> > +            break;
> > +        case 'f':
> > +            format = optarg;
> > +            break;
> 
> Depending on what we decide for Daniel's patches, you may not even want
> a -f here, but always treat this as a new-style command that only takes
> QemuOpts style parsing of a positional parameter.  Right now, I'm
> leaning towards his v3 design (all older sub-commands gain a boolean
> flag that says whether the positional parameters are literal filenames
> or specific QemuOpts strings), but since your subcommand is new, we
> don't have to cater to the older style.

I think the old syntax is friendlier for human users; or at least for
the less experienced ones.

> > +++ b/qemu-img.texi
> > @@ -117,7 +117,7 @@ Skip the creation of the target volume
> >  Command description:
> >  
> >  @table @option
> > address@hidden check [-f @var{fmt}] address@hidden [-r [leaks | all]] [-T 
> > @var{src_cache}] @var{filename}
> > address@hidden check [-q] [-f @var{fmt}] [--force] address@hidden [-r 
> > [leaks | all]] [-T @var{src_cache}] @var{filename}
> 
> Where did -q come from?

From an earlier patch that forgot to update the docs. Should I make that
a separate patch?

> >  
> > address@hidden force-unlock [-f @var{fmt}] @var{filename}
> 
> Okay - most of your patch used the sane spelling; it was just the one
> spot I found that used force_unlock incorrectly.
> 
> > +
> > +Read-write disk images can generally be safely opened only from a single
> > +process at the same time. In order to protect against corruption from
> > +neglecting to follow this rule, qcow2 images are automatically flagged as
> > +in use when they are opened and the flag is removed again on a clean
> > +shutdown.
> > +
> > +However, in cases of an unclean shutdown, the image might be still marked 
> > as in
> > +use so that any further read-write access is prohibited. You can use the
> > address@hidden command to manually remove the in-use flag then.
> > +
> 
> Looks reasonable.  I do think I found enough things, though, that it
> will require a v2 (perhaps rebased on some other patches) before I give R-b.

Kevin

pgpVUhtU0Vdff.pgp
Description: PGP signature