[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/2] Add the blockdev-reopen and blockdev-migrat
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 2/2] Add the blockdev-reopen and blockdev-migrate commands |
Date: |
Mon, 27 Feb 2012 15:57:23 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) |
Kevin Wolf <address@hidden> writes:
> Am 24.02.2012 12:37, schrieb Federico Simoncelli:
[...]
>> diff --git a/blockdev.c b/blockdev.c
>> index 2c132a3..1df2542 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
[...]
>> +void qmp_blockdev_migrate(const char *device, BlockMigrateOp mode,
>> + const char *destination, bool has_new_image_file,
>> + const char *new_image_file, Error **errp)
>> +{
>> + BlockDriverState *bs;
>> + BlockDriver *drv;
>> + int i, j, escape;
>> + char filename[2048];
>> +
>> + bs = bdrv_find(device);
>> + if (!bs) {
>> + error_set(errp, QERR_DEVICE_NOT_FOUND, device);
>> + return;
>> + }
>> + if (bdrv_in_use(bs)) {
>> + error_set(errp, QERR_DEVICE_IN_USE, device);
>> + return;
>> + }
>> +
>> + if (mode == BLOCK_MIGRATE_OP_MIRROR) {
>
> Move into a separate function?
>
>> + drv = bdrv_find_format("blkmirror");
>> + if (!drv) {
>> + error_set(errp, QERR_INVALID_BLOCK_FORMAT, "blkmirror");
>> + return;
>> + }
>> +
>> + if (!has_new_image_file) {
>> + new_image_file = bs->filename;
>> + }
>> +
>> + pstrcpy(filename, sizeof(filename), "blkmirror:");
>> + i = strlen("blkmirror:");
>> +
>> + escape = 0;
>> + for (j = 0; j < strlen(new_image_file); j++) {
>> + 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.
Yes, I did. While I'm no particular friend of QemuOpts, inventing more
ad hoc syntaxes is even worse.
On the other hand, many block drivers already do, often a colon syntax
similar to yours. But not all. I guess consistency is a lost cause
there, as is generality (support for arbitrary filenames). Maybe it's
best to concede defeat, do another ad hoc syntax now, and hope we can
replace the whole -drive mess by something saner eventually.
>> + 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?
>
>> + }
>> +}
>> +
>> static void eject_device(BlockDriverState *bs, int force, Error **errp)
>> {
>> if (bdrv_in_use(bs)) {
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 573b823..ccb1f62 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -886,6 +886,42 @@ Snapshot device, using snapshot file as target if
>> provided
>> ETEXI
>>
>> {
>> + .name = "blkdev_reopen",
>
> NACK on the name. Let's reserve blkdev_*/blockdev_* for the proper API
Yes, please.
> (that we'll release with the qemu version that comes with Hurd 1.0).
/me hides.
> drive_* would align well with the existing drive_add/del commands.
Agree.
[...]
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index d02ee86..c86796a 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -1136,6 +1136,69 @@
>> 'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
>>
>> ##
>> +# @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.
"Deprecated" isn't quite right without a replacement. We could mark it
"experimental" :)
- Re: [Qemu-devel] Live Block Migration using Mirroring, (continued)
- Re: [Qemu-devel] Live Block Migration using Mirroring, Stefan Hajnoczi, 2012/02/23
- Re: [Qemu-devel] Live Block Migration using Mirroring, Stefan Hajnoczi, 2012/02/23
- [Qemu-devel] [PATCH 1/2] Add blkmirror block driver, Federico Simoncelli, 2012/02/24
- [Qemu-devel] [PATCH 2/2] Add the blockdev-reopen and blockdev-migrate commands, Federico Simoncelli, 2012/02/24
- [Qemu-devel] [PATCH 1/2 v2] Add blkmirror block driver, Federico Simoncelli, 2012/02/24
- [Qemu-devel] [PATCH 2/2 v2] Add the blockdev-reopen and blockdev-migrate commands, Federico Simoncelli, 2012/02/24