| Message ID | 1366734308-11724-3-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/23/2013 10:25 AM, Stefan Hajnoczi wrote: > @block-backup > > Start a point-in-time copy of a block device to a new destination. > > @device: the name of the device whose writes should be mirrored. > +++ b/qapi-schema.json > @@ -1715,6 +1715,34 @@ > '*speed': 'int' } } > > ## > +# @block-backup > +# > +# Start a point-in-time copy of a block device to a new destination. > +# > +# @device: the name of the device whose writes should be mirrored. two spaces? > +# > +# @target: the target of the new image. If the file exists, or if it > +# is a device, the existing file/device will be used as the new > +# destination. If it does not exist, a new file will be created. > +# > +# @format: #optional the format of the new destination, default is to > +# probe if @mode is 'existing', else the format of the source > +# > +# @mode: #optional whether and how QEMU should create a new image, default is > +# 'absolute-paths'. > +# > +# @speed: #optional the maximum speed, in bytes per second > +# > +# Returns: nothing on success > +# If @device is not a valid block device, DeviceNotFound > +# > +# Since 1.5 1.6, now > +## > +{ 'command': 'block-backup', > + 'data': { 'device': 'str', 'target': 'str', '*format': 'str', > + '*mode': 'NewImageMode', '*speed': 'int' } } Looks reasonable (since it is closely aligned to drive-mirror).
On 04/26/2013 04:52 PM, Eric Blake wrote: > On 04/23/2013 10:25 AM, Stefan Hajnoczi wrote: >> @block-backup [hit send too soon] Subject line should mention block-backup, not block_backup. >> +## >> +{ 'command': 'block-backup', >> + 'data': { 'device': 'str', 'target': 'str', '*format': 'str', >> + '*mode': 'NewImageMode', '*speed': 'int' } } > > Looks reasonable (since it is closely aligned to drive-mirror). >
On 04/23/2013 10:25 AM, Stefan Hajnoczi wrote: > @block-backup > > Start a point-in-time copy of a block device to a new destination. > > @device: the name of the device whose writes should be mirrored. > > @target: the target of the new image. If the file exists, or if it > is a device, the existing file/device will be used as the new > destination. If it does not exist, a new file will be created. > > @format: #optional the format of the new destination, default is to > probe if @mode is 'existing', else the format of the source > > @mode: #optional whether and how QEMU should create a new image, default is > 'absolute-paths'. > > @speed: #optional the maximum speed, in bytes per second > > Returns: nothing on success > If @device is not a valid block device, DeviceNotFound This starts a new block job type; I assume the existing block-job-cancel and query-block-jobs can track it. I'd really love to see us change 'BlockJobInfo' to use an enum for 'type', instead of its open-coded 'str'. Likewise, the block-job related events in QMP/qmp-events.txt should be updated to refer to the enum instead of also being open-coded 'str'. Will this job be called "backup"?
On Fri, Apr 26, 2013 at 04:58:24PM -0600, Eric Blake wrote: > On 04/23/2013 10:25 AM, Stefan Hajnoczi wrote: > > @block-backup > > > > Start a point-in-time copy of a block device to a new destination. > > > > @device: the name of the device whose writes should be mirrored. > > > > @target: the target of the new image. If the file exists, or if it > > is a device, the existing file/device will be used as the new > > destination. If it does not exist, a new file will be created. > > > > @format: #optional the format of the new destination, default is to > > probe if @mode is 'existing', else the format of the source > > > > @mode: #optional whether and how QEMU should create a new image, default is > > 'absolute-paths'. > > > > @speed: #optional the maximum speed, in bytes per second > > > > Returns: nothing on success > > If @device is not a valid block device, DeviceNotFound > > This starts a new block job type; I assume the existing block-job-cancel > and query-block-jobs can track it. Right. I will update the documentation to be explicit about the block job that this command creates. > I'd really love to see us change 'BlockJobInfo' to use an enum for > 'type', instead of its open-coded 'str'. Likewise, the block-job > related events in QMP/qmp-events.txt should be updated to refer to the > enum instead of also being open-coded 'str'. Since the block job QMP API has been in released I'm not sure changing this is worthwhile. QEMU and libvirt would have to maintain compatibility so the code will just be duplicated. > Will this job be called "backup"? Yes, it is called "backup".
Il 29/04/2013 09:21, Stefan Hajnoczi ha scritto: > > I'd really love to see us change 'BlockJobInfo' to use an enum for > > 'type', instead of its open-coded 'str'. Likewise, the block-job > > related events in QMP/qmp-events.txt should be updated to refer to the > > enum instead of also being open-coded 'str'. > > Since the block job QMP API has been in released I'm not sure changing > this is worthwhile. QEMU and libvirt would have to maintain > compatibility so the code will just be duplicated. I don't think this would change the actual data on the wire. However, it would let libvirt know the supported block job types by introspecting the enum. Paolo
On 04/29/2013 03:27 AM, Paolo Bonzini wrote: > Il 29/04/2013 09:21, Stefan Hajnoczi ha scritto: >>> I'd really love to see us change 'BlockJobInfo' to use an enum for >>> 'type', instead of its open-coded 'str'. Likewise, the block-job >>> related events in QMP/qmp-events.txt should be updated to refer to the >>> enum instead of also being open-coded 'str'. >> >> Since the block job QMP API has been in released I'm not sure changing >> this is worthwhile. QEMU and libvirt would have to maintain >> compatibility so the code will just be duplicated. > > I don't think this would change the actual data on the wire. However, > it would let libvirt know the supported block job types by introspecting > the enum. Until we have introspection, the point is moot. When we have introspection, libvirt would much rather see an enum than a 'str'. I see absolutely no back-compat problem in changing the code to be type-safe prior to the point that introspection is added.
On Mon, Apr 29, 2013 at 09:51:47AM -0600, Eric Blake wrote: > On 04/29/2013 03:27 AM, Paolo Bonzini wrote: > > Il 29/04/2013 09:21, Stefan Hajnoczi ha scritto: > >>> I'd really love to see us change 'BlockJobInfo' to use an enum for > >>> 'type', instead of its open-coded 'str'. Likewise, the block-job > >>> related events in QMP/qmp-events.txt should be updated to refer to the > >>> enum instead of also being open-coded 'str'. > >> > >> Since the block job QMP API has been in released I'm not sure changing > >> this is worthwhile. QEMU and libvirt would have to maintain > >> compatibility so the code will just be duplicated. > > > > I don't think this would change the actual data on the wire. However, > > it would let libvirt know the supported block job types by introspecting > > the enum. > > Until we have introspection, the point is moot. When we have > introspection, libvirt would much rather see an enum than a 'str'. I > see absolutely no back-compat problem in changing the code to be > type-safe prior to the point that introspection is added. Okay, I haven't looked at how QAPI enums are implemented. If we can move from custom strings to enum without breaking existing libvirt, then great! Stefan
diff --git a/blockdev.c b/blockdev.c index 8a1652b..521d999 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1279,6 +1279,98 @@ void qmp_block_commit(const char *device, drive_get_ref(drive_get_by_blockdev(bs)); } +void qmp_block_backup(const char *device, const char *target, + bool has_format, const char *format, + bool has_mode, enum NewImageMode mode, + bool has_speed, int64_t speed, + Error **errp) +{ + BlockDriverState *bs; + BlockDriverState *target_bs; + BlockDriver *proto_drv; + BlockDriver *drv = NULL; + Error *local_err = NULL; + int flags; + uint64_t size; + int ret; + + if (!has_speed) { + speed = 0; + } + if (!has_mode) { + mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS; + } + + bs = bdrv_find(device); + if (!bs) { + error_set(errp, QERR_DEVICE_NOT_FOUND, device); + return; + } + + if (!bdrv_is_inserted(bs)) { + error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); + return; + } + + if (!has_format) { + format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name; + } + if (format) { + drv = bdrv_find_format(format); + if (!drv) { + error_set(errp, QERR_INVALID_BLOCK_FORMAT, format); + return; + } + } + + if (bdrv_in_use(bs)) { + error_set(errp, QERR_DEVICE_IN_USE, device); + return; + } + + flags = bs->open_flags | BDRV_O_RDWR; + + proto_drv = bdrv_find_protocol(target); + if (!proto_drv) { + error_set(errp, QERR_INVALID_BLOCK_FORMAT, format); + return; + } + + bdrv_get_geometry(bs, &size); + size *= 512; + if (mode != NEW_IMAGE_MODE_EXISTING) { + assert(format && drv); + bdrv_img_create(target, format, + NULL, NULL, NULL, size, flags, &local_err, false); + } + + if (error_is_set(&local_err)) { + error_propagate(errp, local_err); + return; + } + + target_bs = bdrv_new(""); + ret = bdrv_open(target_bs, target, NULL, flags, drv); + + if (ret < 0) { + bdrv_delete(target_bs); + error_set(errp, QERR_OPEN_FILE_FAILED, target); + return; + } + + backup_start(bs, target_bs, speed, block_job_cb, bs, &local_err); + if (local_err != NULL) { + bdrv_delete(target_bs); + error_propagate(errp, local_err); + return; + } + + /* Grab a reference so hotplug does not delete the BlockDriverState from + * underneath us. + */ + drive_get_ref(drive_get_by_blockdev(bs)); +} + #define DEFAULT_MIRROR_BUF_SIZE (10 << 20) void qmp_drive_mirror(const char *device, const char *target, diff --git a/qapi-schema.json b/qapi-schema.json index 751d3c2..903d2a5 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1715,6 +1715,34 @@ '*speed': 'int' } } ## +# @block-backup +# +# Start a point-in-time copy of a block device to a new destination. +# +# @device: the name of the device whose writes should be mirrored. +# +# @target: the target of the new image. If the file exists, or if it +# is a device, the existing file/device will be used as the new +# destination. If it does not exist, a new file will be created. +# +# @format: #optional the format of the new destination, default is to +# probe if @mode is 'existing', else the format of the source +# +# @mode: #optional whether and how QEMU should create a new image, default is +# 'absolute-paths'. +# +# @speed: #optional the maximum speed, in bytes per second +# +# Returns: nothing on success +# If @device is not a valid block device, DeviceNotFound +# +# Since 1.5 +## +{ 'command': 'block-backup', + 'data': { 'device': 'str', 'target': 'str', '*format': 'str', + '*mode': 'NewImageMode', '*speed': 'int' } } + +## # @drive-mirror # # Start mirroring a block device's writes to a new destination. diff --git a/qmp-commands.hx b/qmp-commands.hx index 4d65422..ce73e44 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -889,6 +889,12 @@ EQMP }, { + .name = "block-backup", + .args_type = "device:B,target:s,speed:i?,mode:s?,format:s?", + .mhandler.cmd_new = qmp_marshal_input_block_backup, + }, + + { .name = "block-job-set-speed", .args_type = "device:B,speed:o", .mhandler.cmd_new = qmp_marshal_input_block_job_set_speed,
@block-backup Start a point-in-time copy of a block device to a new destination. @device: the name of the device whose writes should be mirrored. @target: the target of the new image. If the file exists, or if it is a device, the existing file/device will be used as the new destination. If it does not exist, a new file will be created. @format: #optional the format of the new destination, default is to probe if @mode is 'existing', else the format of the source @mode: #optional whether and how QEMU should create a new image, default is 'absolute-paths'. @speed: #optional the maximum speed, in bytes per second Returns: nothing on success If @device is not a valid block device, DeviceNotFound Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- blockdev.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ qapi-schema.json | 28 +++++++++++++++++ qmp-commands.hx | 6 ++++ 3 files changed, 126 insertions(+)