Re: [Qemu-devel] [PATCH 2/2] Add the blockdev-reopen and blockdev-migrate commands

[Paolo Bonzini]

On 02/24/2012 01:03 PM, Kevin Wolf wrote:
>> + loop:
>> +            if (!(i < sizeof(filename) - 2)) {
>> +                error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> +                          "new-image-file", "shorter filename");
>> +                return;
>> +            }
>> +
>> +            if (new_image_file[j] == ':' || new_image_file[j] == '\\') {
> 
> Markus suggested that using comma for the separator is better even
> though it requires escaping. It would allow to parse the option string
> with QemuOpts.

Isn't that a bit overengineering for an internal interface?

>> +                if (!escape) {
>> +                    filename[i++] = '\\', escape = 1;
>> +                    goto loop;
>> +                } else {
>> +                    escape = 0;
>> +                }
>> +            }
>> +
>> +            filename[i++] = new_image_file[j];
>> +        }
> 
> Looks like a string helper for qemu-option.c (it contains the parser, so
> keeping the escaping nearby would make sense).
> 
>> +
>> +        if (i + strlen(destination) + 2 > sizeof(filename)) {
>> +            error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> +                      "destination", "shorter filename");
>> +            return;
>> +        }
>> +
>> +        filename[i++] = ':';
>> +        pstrcpy(filename + i, sizeof(filename) - i - 2, destination);
>> +
>> +        change_blockdev_image(device, filename, "blkmirror", false, errp);
>> +    } else if (mode == BLOCK_MIGRATE_OP_STREAM) {
>> +        error_set(errp, QERR_NOT_SUPPORTED);
> 
> Why even define it then?

Because it's the default for the HMP.  Until we have live merging,
mirror mode cannot be consider anything more than a hack.

>>  ##
>> +# @blockdev-reopen
>> +#
>> +# Assigns a new image file to a device.
>> +#
>> +# @device: the name of the device for which we are chainging the image file.
>> +#
>> +# @new-image-file: the target of the new image. If the file doesn't exists 
>> the
>> +#                  command will fail.
>> +#
>> +# @format: #optional the format of the new image, default is 'qcow2'.
>> +#
>> +# Returns: nothing on success
>> +#          If @device is not a valid block device, DeviceNotFound
>> +#          If @new-image-file can't be opened, OpenFileFailed
>> +#          If @format is invalid, InvalidBlockFormat
>> +#
>> +# Since 1.1
>> +##
>> +
>> +{ 'command': 'blockdev-reopen',
>> +  'data': { 'device': 'str', 'new-image-file': 'str', '*format': 'str' } }
> 
> Same consideration on the name.
> 
> Also I think we should immediately mark the command as deprecated (I
> think there is precedence for it, though I can't remember which command
> it was). This is not an interface we'll want to keep long term.

What about blockdev-snapshot-sync/snapshot_blkdev?

Paolo