Patchwork [2/3] block: add block_backup QMP command

login
register
mail settings
Submitter Stefan Hajnoczi
Date April 23, 2013, 4:25 p.m.
Message ID <1366734308-11724-3-git-send-email-stefanha@redhat.com>
Download mbox | patch
Permalink /patch/238956/
State New
Headers show

Comments

Stefan Hajnoczi - April 23, 2013, 4:25 p.m.
@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(+)
Eric Blake - April 26, 2013, 10:52 p.m.
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).
Eric Blake - April 26, 2013, 10:53 p.m.
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).
>
Eric Blake - April 26, 2013, 10:58 p.m.
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"?
Stefan Hajnoczi - April 29, 2013, 7:21 a.m.
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".
Paolo Bonzini - April 29, 2013, 9:27 a.m.
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
Eric Blake - April 29, 2013, 3:51 p.m.
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.
Stefan Hajnoczi - May 1, 2013, 11:55 a.m.
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

Patch

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,