Re: [Qemu-devel] [PATCH 6/8] block: add the drive-reopen command

[Paolo Bonzini]

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