[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 6/8] block: add the drive-reopen command
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH 6/8] block: add the drive-reopen command |
Date: |
Mon, 16 Apr 2012 09:17:54 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20120329 Thunderbird/11.0.1 |
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
- [Qemu-devel] [RFC PATCH 0/8] job-based mirroring implementation, Paolo Bonzini, 2012/04/13
- [Qemu-devel] [PATCH 2/8] block: allow interrupting a co_sleep_ns, Paolo Bonzini, 2012/04/13
- [Qemu-devel] [PATCH 1/8] block: introduce new dirty bitmap functionality, Paolo Bonzini, 2012/04/13
- [Qemu-devel] [PATCH 3/8] block: allow doing I/O in a job after cancellation, Paolo Bonzini, 2012/04/13
- [Qemu-devel] [PATCH 5/8] block: add drive-mirror command, Paolo Bonzini, 2012/04/13
- [Qemu-devel] [PATCH 6/8] block: add the drive-reopen command, Paolo Bonzini, 2012/04/13
- [Qemu-devel] [PATCH 4/8] block: add mirror job, Paolo Bonzini, 2012/04/13
- [Qemu-devel] [PATCH 8/8] docs: add mirroring to live block operations, Paolo Bonzini, 2012/04/13
- Re: [Qemu-devel] [PATCH 8/8] docs: add mirroring to live block operations, Eric Blake, 2012/04/13
- Re: [Qemu-devel] [PATCH 8/8] docs: add mirroring to live block operations, Paolo Bonzini, 2012/04/13
- Re: [Qemu-devel] [PATCH 8/8] docs: add mirroring to live block operations, Eric Blake, 2012/04/13
- Re: [Qemu-devel] [PATCH 8/8] docs: add mirroring to live block operations, Paolo Bonzini, 2012/04/13
- Re: [Qemu-devel] [PATCH 8/8] docs: add mirroring to live block operations, Eric Blake, 2012/04/13
- Re: [Qemu-devel] [PATCH 8/8] docs: add mirroring to live block operations, Paolo Bonzini, 2012/04/16
[Qemu-devel] [PATCH 7/8] block: add witness argument to drive-reopen, Paolo Bonzini, 2012/04/13