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

[Max Reitz <mreitz@redhat.com>]

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 <mreitz@redhat.com>
>> ---
>>   
>> +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.
>> +
>> +@item amend [-f @var{fmt}] -o @var{options} @var{filename}
>> +
>> +Amends the image format specific @var{options} for the image file
>> +@var{filename}. 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