Kevin Wolf <kw...@redhat.com> 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" :)