Re: [Qemu-devel] [PATCH v2 1/3] block: Image file option amendment

[Eric Blake]

On 08/29/2013 05:20 AM, Max Reitz wrote:
> This patch adds the "amend" option to qemu-img which allows changing
> image options on existing image files. It also adds the generic bdrv
> implementation which is basically just a wrapper for the image format
> specific function.
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  
> +static int img_amend(int argc, char **argv)
> +{
> +    int c, ret = 0;
> +    char *options = NULL;
> +    QEMUOptionParameter *create_options = NULL, *options_param = NULL;
> +    const char *fmt = NULL, *filename;
> +    bool quiet = false;
> +    BlockDriverState *bs = NULL;
> +
> +    for (;;) {
> +        c = getopt(argc, argv, "h?qf:o:");

? is not usually listed in the string to getopt() (doing so is not
portable to POSIX).  Besides, use of an unknown option (such as -x) will
get mapped to '?' by getopt anyways, so your goal...

> +        if (c == -1) {
> +            break;
> +        }
> +
> +        switch (c) {
> +            case 'h':
> +            case '?':
> +                help();
> +                break;

...of printing help on unknown options is already met without the need
for an explicit '-?' option.

> +
> +    filename = argv[argc - 1];
> +
> +    bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR, true, 
> quiet);
> +    if (!bs) {
> +        error_report("Could not open image.");

We generally avoid trailing '.' in error messages; it might also be nice
to report WHICH image could not be opened.

> +    options_param = parse_option_parameters(options, create_options,
> +            options_param);
> +    if (options_param == NULL) {
> +        error_report("Invalid options for file format '%s'.", fmt);

again, no trailing '.'

> +++ b/qemu-img.texi
> @@ -282,6 +282,11 @@ sizes accordingly.  Failure to do so will result in data 
> loss!
>  After using this command to grow a disk image, you must use file system and
>  partitioning tools inside the VM to actually begin using the new space on the
>  device.
> +
> address@hidden amend [-f @var{fmt}] -o @var{options} @var{filename}
> +
> +Amends the image format specific @var{options} for the image file
> address@hidden Only the format @code{qcow2} supports this.

We might add support for other file formats in the future; we'd have to
remember to update this sentence at that time.  Could you use a more
generic statement, such as "not all file formats support this", so we
don't end up with stale docs if we forget to touch it down the road?

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

signature.asc
Description: OpenPGP digital signature