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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 7589 bytes --]

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

>  
>  /*
> 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)

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

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

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

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

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

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

> +++ b/qemu-img.texi
> @@ -117,7 +117,7 @@ Skip the creation of the target volume
>  Command description:
>  
>  @table @option
> -@item check [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] @var{filename}
> +@item check [-q] [-f @var{fmt}] [--force] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] @var{filename}

Where did -q come from?

>  
> +@item 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
> +@code{force-unlock} 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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]