Re: [Qemu-devel] [PATCH 1/3] FVD: Added support for 'qemu-img update'

[Stefan Hajnoczi <stefanha@gmail.com>]

On Fri, Jan 21, 2011 at 05:19:13PM -0500, Chunqiang Tang wrote:
> This patch adds the 'update' command to qemu-img. FVD stores various
> image-specific configurable parameters in the image header. A user can use
> 'qemu-img update' to modify those parameters and accordingly controls FVD's
> runtime behavior. This command may also be leveraged by other block device
> drivers, e.g., to set the size of the in-memory metadata cache. Currently
> those parameters are hard-coded in a one-size-fit-all manner.

There's a high risk that users will try this command while the VM is
running.  A safe-guard is needed here in order to avoid corrupting the
image.

Please use qemu-option.h instead of int argc, char **argv just like
qemu-img create -o does.

Finally, is this interface really necessary?  As a developer it can be
useful to tweak image values (in QED I actually have a free-standing
tool that can query and manipulate image internals).  But should users
need to micromanage every image file in order to achieve desired
functionality/performance?  What's the real need here?

> diff --git a/qemu-img.c b/qemu-img.c
> index afd9ed2..5f35c4d 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1054,6 +1054,49 @@ static int img_info(int argc, char **argv)
>      return 0;
>  }
>  
> +static int img_update(int argc, char **argv)
> +{
> +    int c;
> +    const char *filename, *fmt;
> +    BlockDriverState *bs;
> +
> +    fmt = NULL;
> +    for(;;) {
> +        c = getopt(argc, argv, "f:h");
> +        if (c == -1)
> +            break;

{}, see CODING_STYLE and HACKING.

> +        switch(c) {
> +        case 'h':
> +            help();
> +            break;
> +        case 'f':
> +            fmt = optarg;
> +            break;
> +        }
> +    }
> +    if (optind >= argc)
> +        help();

{}

> +    filename = argv[optind++];
> +    bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING
> +                       | BDRV_O_RDWR);
> +    if (!bs) {
> +        return 1;
> +    }
> +
> +    if (bs->drv->bdrv_update) {
> +        bs->drv->bdrv_update(bs, argc-optind, &argv[optind]);
> +    }
> +    else {

} else {, see CODING_STYLE

> +        char fmt_name[128];
> +        bdrv_get_format(bs, fmt_name, sizeof(fmt_name));
> +        error_report ("The 'update' command is not supported for "
> +                      "the '%s' image format.", fmt_name);

Return value should be 1?

Stefan