| Message ID | 1334334198-30899-7-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/13/2012 10:23 AM, Paolo Bonzini wrote: > From: Federico Simoncelli <fsimonce@redhat.com> > > Signed-off-by: Federico Simoncelli <fsimonce@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > blockdev.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > hmp-commands.hx | 16 ++++++++++++ > hmp.c | 11 ++++++++ > hmp.h | 1 + > qapi-schema.json | 22 ++++++++++++++++ > qmp-commands.hx | 30 ++++++++++++++++++++++ > 6 files changed, 155 insertions(+) > > > +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? > + > + 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? > + > + bdrv_close(bs); > + ret = bdrv_open(bs, new_image_file, flags, drv); Here's where it is hairy, and a future patch to do things right in a transaction would be nice (open the new chain in prepare phase, then close the old chain in commit phase, all while dealing with any possible overlap between the two chains), but I can live with this for now. > + /* > + * 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? > +++ 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. > +++ 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/ > +# > +# @format: #optional the format of the new image, default is 'qcow2'. again, default is to probe, not hard-code qcow2. > +SQMP > +drive-reopen > +------------ > + > +Assigns a new image file to a device. Except extremely rare cases where the s/Except extremely/Except in extremely/ > +guest is expecting the drive to change its content, the new image should > +contain the same data of the current one. One use case is to terminate > +a drive-mirror command. > +
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
Am 16.04.2012 09:17, schrieb Paolo Bonzini: >>> + /* >>> + * 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. It happens more or less automatically. After a failed bdrv_open() you have bs->drv = NULL, which makes any subsequent requests fail. This way you trigger the rerror/werror paths indirectly. Kevin
diff --git a/blockdev.c b/blockdev.c index ce2f2f6..08953fa 100644 --- a/blockdev.c +++ b/blockdev.c @@ -659,6 +659,81 @@ void do_commit(Monitor *mon, const QDict *qdict) } } +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]; + + 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; + + if (format) { + drv = bdrv_find_whitelisted_format(format); + if (!drv) { + error_set(errp, QERR_INVALID_BLOCK_FORMAT, format); + return; + } + } + + proto_drv = bdrv_find_protocol(new_image_file); + if (!proto_drv) { + error_set(errp, QERR_INVALID_BLOCK_FORMAT, format); + return; + } + + bdrv_drain_all(); + if (!bdrv_is_read_only(bs) && bdrv_is_inserted(bs)) { + if (bdrv_flush(bs)) { + error_set(errp, QERR_IO_ERROR); + return; + } + } + + bdrv_close(bs); + ret = bdrv_open(bs, new_image_file, flags, drv); + /* + * 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); + } + } +} + +void qmp_drive_reopen(const char *device, const char *new_image_file, + bool has_format, const char *format, Error **errp) +{ + BlockDriverState *bs; + + bs = bdrv_find(device); + if (!bs) { + error_set(errp, QERR_DEVICE_NOT_FOUND, device); + return; + } + if (bs->job) { + block_job_cancel_sync(bs->job); + } + change_blockdev_image(bs, new_image_file, + has_format ? format : NULL, errp); +} + static void blockdev_do_action(int kind, void *data, Error **errp) { BlockdevAction action; diff --git a/hmp-commands.hx b/hmp-commands.hx index 36f4ef9..7c5f090 100644 --- a/hmp-commands.hx +++ 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", + .mhandler.cmd = hmp_drive_reopen, + }, + +STEXI +@item drive_reopen +@findex drive_reopen +Assigns a new image file to a device. +ETEXI + + { .name = "drive_add", .args_type = "pci_addr:s,opts:s", .params = "[[<domain>:]<bus>:]<slot>\n" diff --git a/hmp.c b/hmp.c index e7290c9..28697ec 100644 --- a/hmp.c +++ b/hmp.c @@ -737,6 +737,17 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict) hmp_handle_error(mon, &errp); } +void hmp_drive_reopen(Monitor *mon, const QDict *qdict) +{ + const char *device = qdict_get_str(qdict, "device"); + const char *filename = qdict_get_str(qdict, "new-image-file"); + const char *format = qdict_get_try_str(qdict, "format"); + Error *errp = NULL; + + qmp_drive_reopen(device, filename, !!format, format, &errp); + hmp_handle_error(mon, &errp); +} + void hmp_migrate_cancel(Monitor *mon, const QDict *qdict) { qmp_migrate_cancel(NULL); diff --git a/hmp.h b/hmp.h index 64bcaa6..5f6976a 100644 --- a/hmp.h +++ b/hmp.h @@ -49,6 +49,7 @@ void hmp_balloon(Monitor *mon, const QDict *qdict); void hmp_block_resize(Monitor *mon, const QDict *qdict); void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict); void hmp_drive_mirror(Monitor *mon, const QDict *qdict); +void hmp_drive_reopen(Monitor *mon, const QDict *qdict); void hmp_migrate_cancel(Monitor *mon, const QDict *qdict); void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict); void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict); diff --git a/qapi-schema.json b/qapi-schema.json index c772bc3..0bf3a25 100644 --- a/qapi-schema.json +++ 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. +# +# @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': 'drive-reopen', + 'data': { 'device': 'str', 'new-image-file': 'str', '*format': 'str' } } + +## # @human-monitor-command: # # Execute a command on the human monitor and return the output. diff --git a/qmp-commands.hx b/qmp-commands.hx index a51125e..6ea0ef5 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -828,6 +828,36 @@ Example: EQMP { + .name = "drive-reopen", + .args_type = "device:B,new-image-file:s,format:s?", + .mhandler.cmd_new = qmp_marshal_input_drive_reopen, + }, + +SQMP +drive-reopen +------------ + +Assigns a new image file to a device. Except extremely rare cases where the +guest is expecting the drive to change its content, the new image should +contain the same data of the current one. One use case is to terminate +a drive-mirror command. + +Arguments: + +- "device": device name to operate on (json-string) +- "new-image-file": name of new image file (json-string) +- "format": format of new image (json-string, optional) + +Example: + +-> { "execute": "drive-reopen", "arguments": {"device": "ide-hd0", + "new-image-file": "/some/place/my-image", + "format": "qcow2" } } +<- { "return": {} } + +EQMP + + { .name = "balloon", .args_type = "value:M", .mhandler.cmd_new = qmp_marshal_input_balloon,