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

[Max Reitz]

Am 29.08.2013 14:38, schrieb 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.
Ah, okay, thanks.

> > +
> > +    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.
Yes, I'll fix it.

> > +    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?
Of course.


Max