Il 14/04/2012 00:01, Eric Blake ha scritto:
>> +static void change_blockdev_image(BlockDriverState *bs, const char
>> *new_image_file,
>> + const char *format, Error **errp)
>> +{
>> + BlockDriver *old_drv, *proto_drv;
>> + BlockDriver *drv = NULL;
>> + int ret = 0;
>> + int flags;
>> + char old_filename[1024];
>
> Hard-coded limit. Is this going to bite us later, or are we stuck with
> this limit in other places for other reasons? Why a magic number
> instead of a macro name or enum value?
In BlockDriverState:
char filename[1024];
>> +
>> + if (bdrv_in_use(bs)) {
>> + error_set(errp, QERR_DEVICE_IN_USE, bs->device_name);
>> + return;
>> + }
>> +
>> + pstrcpy(old_filename, sizeof(old_filename), bs->filename);
>> +
>> + old_drv = bs->drv;
>> + flags = bs->open_flags;
>
> Should we be asserting that flags does not contain BDRV_O_NO_BACKING, so
> that we know that we are reopening the entire chain?
No, I think it's okay (if for any reason the source had
BDRV_O_NO_BACKING) to use it also for the new image.
>> + /*
>> + * If reopening the image file we just created fails, fall back
>> + * and try to re-open the original image. If that fails too, we
>> + * are in serious trouble.
>> + */
>> + if (ret != 0) {
>> + ret = bdrv_open(bs, old_filename, flags, old_drv);
>> + if (ret != 0) {
>> + error_set(errp, QERR_OPEN_FILE_FAILED, old_filename);
>> + } else {
>> + error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
>> + }
>> + }
>
> In that worst-case scenario of failing to reopen the old file, should we
> be halting the domain and/or propagating an event up to the user,
> similar to how we behave on ENOSPC errors?
Probably yes, but it's made more complex because the rerror/werror
arguments are only known by the emulated device, not by the disk. I
(and Federico before me) just copied the existing non-handling in live
snapshots.
>> +++ b/hmp-commands.hx
>> @@ -922,6 +922,22 @@ using the specified target.
>> ETEXI
>>
>> {
>> + .name = "drive_reopen",
>> + .args_type = "device:B,new-image-file:s,format:s?",
>> + .params = "device new-image-file [format]",
>> + .help = "Assigns a new image file to a device.\n\t\t\t"
>> + "The image will be opened using the format\n\t\t\t"
>> + "specified or 'qcow2' by default.\n\t\t\t",
>
> Really? I though if you didn't provide format, you get probing, not
> forced qcow2.
Right.
>> +++ b/qapi-schema.json
>> @@ -1217,6 +1217,28 @@
>> '*mode': 'NewImageMode'} }
>>
>> ##
>> +# @drive-reopen
>> +#
>> +# Assigns a new image file to a device.
>> +#
>> +# @device: the name of the device for which we are changing the image file.
>> +#
>> +# @new-image-file: the target of the new image. If the file doesn't exists
>> the
>> +# command will fail.
>
> s/exists/exist/
Thanks.
>> +#
>> +# @format: #optional the format of the new image, default is 'qcow2'.
>
> again, default is to probe, not hard-code qcow2.
Yes.
Paolo
