[Qemu-devel] [PATCH 0/1] Allow passing BlockdevOptions to blockdev-snapshot-sync

[Qemu-devel] [PATCH 0/1] Allow passing BlockdevOptions to blockdev-snapshot-sync

[Alberto Garcia]

Here's my first attempt to solve the problem explained here:

   https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02847.html

In short: there's no way to pass options to snapshots created using
blockdev-snapshot-sync.

This patch simply allows passing a BlockdevOptions structure to that
command. I'm not 100% happy with the API because there's some overlap
with a couple of parameters, but it's the simplest I could think of.

This need to be applied on top of this series from Max Reitz:

   https://lists.gnu.org/archive/html/qemu-block/2015-08/msg00189.html

Regards,

Berto

Alberto Garcia (1):
  block: Allow passing BlockdevOptions to blockdev-snapshot-sync

 blockdev.c           | 59 ++++++++++++++++++++++++++++++++++++++++++++++------
 hmp.c                |  2 +-
 qapi/block-core.json |  9 +++++++-
 qmp-commands.hx      |  3 ++-
 4 files changed, 64 insertions(+), 9 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH 1/1] block: Allow passing BlockdevOptions to blockdev-snapshot-sync

[Alberto Garcia]

Snapshots created using blockdev-snapshot-sync are currently opened
using their default options, not even inheriting those from the images
they are based on.

This patch extends the command by adding an 'options' parameter that
takes a BlockdevOptions structure. Since some of the options that can
be specified overlap with the parameters of blockdev-snapshot-sync,
the following checks are made:

- The 'driver' field must match the format specified for the snapshot.
- The 'node-name' field and the 'snapshot-node-name' parameters cannot
  be specified at the same time.
- The 'file' field can only contain an empty string, since it overlaps
  with the 'snapshot-file' parameter.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c           | 59 ++++++++++++++++++++++++++++++++++++++++++++++------
 hmp.c                |  2 +-
 qapi/block-core.json |  9 +++++++-
 qmp-commands.hx      |  3 ++-
 4 files changed, 64 insertions(+), 9 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 4a1fc2e..7e904f3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1070,7 +1070,9 @@ void qmp_blockdev_snapshot_sync(bool has_device, const char *device,
                                 bool has_snapshot_node_name,
                                 const char *snapshot_node_name,
                                 bool has_format, const char *format,
-                                bool has_mode, NewImageMode mode, Error **errp)
+                                bool has_mode, NewImageMode mode,
+                                bool has_options, BlockdevOptions *options,
+                                Error **errp)
 {
     BlockdevSnapshot snapshot = {
         .has_device = has_device,
@@ -1084,6 +1086,8 @@ void qmp_blockdev_snapshot_sync(bool has_device, const char *device,
         .format = (char *) format,
         .has_mode = has_mode,
         .mode = mode,
+        .has_options = has_options,
+        .options = options,
     };
     blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC,
                        &snapshot, errp);
@@ -1504,6 +1508,48 @@ static void external_snapshot_prepare(BlkTransactionState *common,
 
     flags = state->old_bs->open_flags;
 
+    if (action->blockdev_snapshot_sync->has_options) {
+        QObject *obj;
+        QmpOutputVisitor *ov = qmp_output_visitor_new();
+        visit_type_BlockdevOptions(qmp_output_get_visitor(ov),
+                                   &action->blockdev_snapshot_sync->options,
+                                   NULL, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            qmp_output_visitor_cleanup(ov);
+            return;
+        }
+
+        obj = qmp_output_get_qobject(ov);
+        options = qobject_to_qdict(obj);
+        qmp_output_visitor_cleanup(ov);
+
+        if (strcmp(format, qdict_get_str(options, "driver"))) {
+            error_setg(errp, "Can't specify two different image formats");
+            goto fail;
+        }
+
+        if (has_snapshot_node_name && qdict_haskey(options, "node-name")) {
+            error_setg(errp, "Can't specify a node name twice");
+            goto fail;
+        }
+
+        obj = qdict_get(options, "file");
+        if (obj != NULL) {
+            QString *str = qobject_to_qstring(obj);
+            if (!str || strcmp(qstring_get_str(str), "")) {
+                error_setg(errp, "The 'file' field must be empty");
+                goto fail;
+            }
+            qdict_del(options, "file");
+        }
+
+        qdict_flatten(options);
+    } else {
+        options = qdict_new();
+        qdict_put(options, "driver", qstring_from_str(format));
+    }
+
     /* create new image w/backing file */
     if (mode != NEW_IMAGE_MODE_EXISTING) {
         bdrv_img_create(new_image_file, format,
@@ -1512,19 +1558,15 @@ static void external_snapshot_prepare(BlkTransactionState *common,
                         NULL, -1, flags, &local_err, false);
         if (local_err) {
             error_propagate(errp, local_err);
-            return;
+            goto fail;
         }
     }
 
-    options = qdict_new();
     if (has_snapshot_node_name) {
         qdict_put(options, "node-name",
                   qstring_from_str(snapshot_node_name));
     }
-    qdict_put(options, "driver", qstring_from_str(format));
 
-    /* TODO Inherit bs->options or only take explicit options with an
-     * extended QMP command? */
     assert(state->new_bs == NULL);
     ret = bdrv_open(&state->new_bs, new_image_file, NULL, options,
                     flags | BDRV_O_NO_BACKING, &local_err);
@@ -1532,6 +1574,11 @@ static void external_snapshot_prepare(BlkTransactionState *common,
     if (ret != 0) {
         error_propagate(errp, local_err);
     }
+
+    return;
+
+fail:
+    QDECREF(options);
 }
 
 static void external_snapshot_commit(BlkTransactionState *common)
diff --git a/hmp.c b/hmp.c
index dcc66f1..180a7f6 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1115,7 +1115,7 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
     qmp_blockdev_snapshot_sync(true, device, false, NULL,
                                filename, false, NULL,
                                !!format, format,
-                               true, mode, &err);
+                               true, mode, false, NULL, &err);
     hmp_handle_error(mon, &err);
 }
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7b2efb8..5eb111e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -697,11 +697,18 @@
 #
 # @mode: #optional whether and how QEMU should create a new image, default is
 #        'absolute-paths'.
+#
+# @options: #optional options for the new device, with the following
+#           restrictions for the fields: 'driver' must match the value
+#           of @format, 'node-name' and @snapshot-node-name cannot be
+#           specified at the same time, and 'file' can only contain an
+#           empty string (Since 2.5)
 ##
 { 'struct': 'BlockdevSnapshot',
   'data': { '*device': 'str', '*node-name': 'str',
             'snapshot-file': 'str', '*snapshot-node-name': 'str',
-            '*format': 'str', '*mode': 'NewImageMode' } }
+            '*format': 'str', '*mode': 'NewImageMode',
+            '*options': 'BlockdevOptions' } }
 
 ##
 # @DriveBackup
diff --git a/qmp-commands.hx b/qmp-commands.hx
index ba630b1..bf45e31 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1394,7 +1394,7 @@ EQMP
 
     {
         .name       = "blockdev-snapshot-sync",
-        .args_type  = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?",
+        .args_type  = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?,options:q?",
         .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,
     },
 
@@ -1417,6 +1417,7 @@ Arguments:
 - "mode": whether and how QEMU should create the snapshot file
   (NewImageMode, optional, default "absolute-paths")
 - "format": format of new image (json-string, optional)
+- "options": options for the new image (json-dict)
 
 Example:
 
-- 
2.5.0

Re: [Qemu-devel] [PATCH 1/1] block: Allow passing BlockdevOptions to blockdev-snapshot-sync

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 8695 bytes --]

On 31.08.2015 12:00, Alberto Garcia wrote:
> Snapshots created using blockdev-snapshot-sync are currently opened
> using their default options, not even inheriting those from the images
> they are based on.
> 
> This patch extends the command by adding an 'options' parameter that
> takes a BlockdevOptions structure. Since some of the options that can
> be specified overlap with the parameters of blockdev-snapshot-sync,
> the following checks are made:
> 
> - The 'driver' field must match the format specified for the snapshot.
> - The 'node-name' field and the 'snapshot-node-name' parameters cannot
>   be specified at the same time.
> - The 'file' field can only contain an empty string, since it overlaps
>   with the 'snapshot-file' parameter.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c           | 59 ++++++++++++++++++++++++++++++++++++++++++++++------
>  hmp.c                |  2 +-
>  qapi/block-core.json |  9 +++++++-
>  qmp-commands.hx      |  3 ++-
>  4 files changed, 64 insertions(+), 9 deletions(-)

Design question: Would it make sense to instead add a "reference" mode
to blockdev-snapshot-sync where you can specify a BDS's node-name
instead of snapshot-file to use an existing BDS as the new top layer,
ideally an empty one?

What we'd then need is a QMP command for creating images. But as far as
I know we'll need that anyway sooner or later...


Comments on the patch itself below.

> diff --git a/blockdev.c b/blockdev.c
> index 4a1fc2e..7e904f3 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1070,7 +1070,9 @@ void qmp_blockdev_snapshot_sync(bool has_device, const char *device,
>                                  bool has_snapshot_node_name,
>                                  const char *snapshot_node_name,
>                                  bool has_format, const char *format,
> -                                bool has_mode, NewImageMode mode, Error **errp)
> +                                bool has_mode, NewImageMode mode,
> +                                bool has_options, BlockdevOptions *options,
> +                                Error **errp)
>  {
>      BlockdevSnapshot snapshot = {
>          .has_device = has_device,
> @@ -1084,6 +1086,8 @@ void qmp_blockdev_snapshot_sync(bool has_device, const char *device,
>          .format = (char *) format,
>          .has_mode = has_mode,
>          .mode = mode,
> +        .has_options = has_options,
> +        .options = options,
>      };
>      blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC,
>                         &snapshot, errp);
> @@ -1504,6 +1508,48 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>  
>      flags = state->old_bs->open_flags;
>  
> +    if (action->blockdev_snapshot_sync->has_options) {
> +        QObject *obj;
> +        QmpOutputVisitor *ov = qmp_output_visitor_new();
> +        visit_type_BlockdevOptions(qmp_output_get_visitor(ov),
> +                                   &action->blockdev_snapshot_sync->options,
> +                                   NULL, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            qmp_output_visitor_cleanup(ov);
> +            return;
> +        }
> +
> +        obj = qmp_output_get_qobject(ov);
> +        options = qobject_to_qdict(obj);
> +        qmp_output_visitor_cleanup(ov);
> +
> +        if (strcmp(format, qdict_get_str(options, "driver"))) {

With has_format == false, format will be set to "qcow2" by default. So,
if the user does not specify the format explicitly, the "driver" field
has to be set to "qcow2".

I'd rather make specifying @format and @options exclusive, and if
@options has been specified, its "driver" field should override the
"format" default.

> +            error_setg(errp, "Can't specify two different image formats");
> +            goto fail;
> +        }
> +
> +        if (has_snapshot_node_name && qdict_haskey(options, "node-name")) {
> +            error_setg(errp, "Can't specify a node name twice");
> +            goto fail;
> +        }
> +
> +        obj = qdict_get(options, "file");
> +        if (obj != NULL) {
> +            QString *str = qobject_to_qstring(obj);
> +            if (!str || strcmp(qstring_get_str(str), "")) {
> +                error_setg(errp, "The 'file' field must be empty");
> +                goto fail;
> +            }
> +            qdict_del(options, "file");
> +        }

And the "backing" field must be NULL.

> +
> +        qdict_flatten(options);
> +    } else {
> +        options = qdict_new();
> +        qdict_put(options, "driver", qstring_from_str(format));
> +    }
> +
>      /* create new image w/backing file */
>      if (mode != NEW_IMAGE_MODE_EXISTING) {
>          bdrv_img_create(new_image_file, format,
> @@ -1512,19 +1558,15 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>                          NULL, -1, flags, &local_err, false);
>          if (local_err) {
>              error_propagate(errp, local_err);
> -            return;
> +            goto fail;
>          }
>      }
>  
> -    options = qdict_new();
>      if (has_snapshot_node_name) {
>          qdict_put(options, "node-name",
>                    qstring_from_str(snapshot_node_name));
>      }
> -    qdict_put(options, "driver", qstring_from_str(format));
>  
> -    /* TODO Inherit bs->options or only take explicit options with an
> -     * extended QMP command? */
>      assert(state->new_bs == NULL);
>      ret = bdrv_open(&state->new_bs, new_image_file, NULL, options,
>                      flags | BDRV_O_NO_BACKING, &local_err);
> @@ -1532,6 +1574,11 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>      if (ret != 0) {
>          error_propagate(errp, local_err);
>      }
> +
> +    return;
> +
> +fail:
> +    QDECREF(options);
>  }
>  
>  static void external_snapshot_commit(BlkTransactionState *common)
> diff --git a/hmp.c b/hmp.c
> index dcc66f1..180a7f6 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1115,7 +1115,7 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
>      qmp_blockdev_snapshot_sync(true, device, false, NULL,
>                                 filename, false, NULL,
>                                 !!format, format,
> -                               true, mode, &err);
> +                               true, mode, false, NULL, &err);
>      hmp_handle_error(mon, &err);
>  }
>  
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7b2efb8..5eb111e 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -697,11 +697,18 @@
>  #
>  # @mode: #optional whether and how QEMU should create a new image, default is
>  #        'absolute-paths'.
> +#
> +# @options: #optional options for the new device, with the following
> +#           restrictions for the fields: 'driver' must match the value
> +#           of @format,

As said above, I'd rather make specifying both @options and @format
exclusive.

Maybe there is even some QAPI magic to enforce that (and for
'node-name', too), I don't know...

Max

>                          'node-name' and @snapshot-node-name cannot be
> +#           specified at the same time, and 'file' can only contain an
> +#           empty string (Since 2.5)
>  ##
>  { 'struct': 'BlockdevSnapshot',
>    'data': { '*device': 'str', '*node-name': 'str',
>              'snapshot-file': 'str', '*snapshot-node-name': 'str',
> -            '*format': 'str', '*mode': 'NewImageMode' } }
> +            '*format': 'str', '*mode': 'NewImageMode',
> +            '*options': 'BlockdevOptions' } }
>  
>  ##
>  # @DriveBackup
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index ba630b1..bf45e31 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1394,7 +1394,7 @@ EQMP
>  
>      {
>          .name       = "blockdev-snapshot-sync",
> -        .args_type  = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?",
> +        .args_type  = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?,options:q?",
>          .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,
>      },
>  
> @@ -1417,6 +1417,7 @@ Arguments:
>  - "mode": whether and how QEMU should create the snapshot file
>    (NewImageMode, optional, default "absolute-paths")
>  - "format": format of new image (json-string, optional)
> +- "options": options for the new image (json-dict)
>  
>  Example:
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 1/1] block: Allow passing BlockdevOptions to blockdev-snapshot-sync

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3255 bytes --]

On 08/31/2015 01:53 PM, Max Reitz wrote:

> Design question: Would it make sense to instead add a "reference" mode
> to blockdev-snapshot-sync where you can specify a BDS's node-name
> instead of snapshot-file to use an existing BDS as the new top layer,
> ideally an empty one?

Indeed - then blockdev-add can be used to create an unattached BDS (with
all appropriate options), and blockdev-snapshot-sync would then attach
that BDS as the snapshot-file that wraps an existing BDS (without
needing to worry about options).

> 
> What we'd then need is a QMP command for creating images. But as far as
> I know we'll need that anyway sooner or later...

Can't blockdev-add already be used for that (at least, for supported
file types)? If not, what would it take to get it there?

> 
> 
> Comments on the patch itself below.
> 

> 
> With has_format == false, format will be set to "qcow2" by default. So,
> if the user does not specify the format explicitly, the "driver" field
> has to be set to "qcow2".
> 
> I'd rather make specifying @format and @options exclusive, and if
> @options has been specified, its "driver" field should override the
> "format" default.
> 

>> +++ b/qapi/block-core.json
>> @@ -697,11 +697,18 @@
>>  #
>>  # @mode: #optional whether and how QEMU should create a new image, default is
>>  #        'absolute-paths'.
>> +#
>> +# @options: #optional options for the new device, with the following
>> +#           restrictions for the fields: 'driver' must match the value
>> +#           of @format,
> 
> As said above, I'd rather make specifying both @options and @format
> exclusive.
> 
> Maybe there is even some QAPI magic to enforce that (and for
> 'node-name', too), I don't know...

Not that I know of at the moment, but not to say we can't add some.  The
closest we can get is with a flat union, but that requires a
non-optional discriminator field.  Maybe we can tweak qapi to make the
discriminator optional (with a default value).  Thankfully, it sounds
like Markus' work on introspection would at least let management apps
learn about a new 'options' argument.

>>                          'node-name' and @snapshot-node-name cannot be
>> +#           specified at the same time, and 'file' can only contain an
>> +#           empty string (Since 2.5)

I really don't like the idea of requiring the user to pass in an empty
string. Can we make the field optional instead, and require it to be
omitted in the context where it would otherwise need to be empty, while
still requiring it to be present in existing clients that weren't
prepared for it to be optional?  It also feels a bit ad hoc that you
describe that driver/format must be identical, but that other fields
must be mutually exclusive or the empty string; consistency would argue
that since you already handle duplication of driver/format by requiring
them to be the same, that you should also allow duplication and
validation of identical other fields that overlap.  (But even better
would be finding a way to not allow overlapping fields to appear in the
first place).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/1] block: Allow passing BlockdevOptions to blockdev-snapshot-sync

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1403 bytes --]

On 31.08.2015 22:05, Eric Blake wrote:
> On 08/31/2015 01:53 PM, Max Reitz wrote:
> 
>> Design question: Would it make sense to instead add a "reference" mode
>> to blockdev-snapshot-sync where you can specify a BDS's node-name
>> instead of snapshot-file to use an existing BDS as the new top layer,
>> ideally an empty one?
> 
> Indeed - then blockdev-add can be used to create an unattached BDS (with
> all appropriate options), and blockdev-snapshot-sync would then attach
> that BDS as the snapshot-file that wraps an existing BDS (without
> needing to worry about options).
> 
>>
>> What we'd then need is a QMP command for creating images. But as far as
>> I know we'll need that anyway sooner or later...
> 
> Can't blockdev-add already be used for that (at least, for supported
> file types)? If not, what would it take to get it there?

It would take a blockdev-create-image QMP command. :-)

blockdev-add only opens existing images, blockdev-create-image would
then create these so they can be opened using blockdev-add.

Similar to blockdev-add, it would probably have a single parameter, but
it'd be of a different type, called e.g. BlockdevCreateOptions, since it
has to reflect the creation options instead of the runtime options for
opening existing images. For instance, for qcow2 you could set the
refcount-bits value, but not the L2 cache size.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 1/1] block: Allow passing BlockdevOptions to blockdev-snapshot-sync

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1769 bytes --]

Am 31.08.2015 um 22:12 hat Max Reitz geschrieben:
> On 31.08.2015 22:05, Eric Blake wrote:
> > On 08/31/2015 01:53 PM, Max Reitz wrote:
> > 
> >> Design question: Would it make sense to instead add a "reference" mode
> >> to blockdev-snapshot-sync where you can specify a BDS's node-name
> >> instead of snapshot-file to use an existing BDS as the new top layer,
> >> ideally an empty one?
> > 
> > Indeed - then blockdev-add can be used to create an unattached BDS (with
> > all appropriate options), and blockdev-snapshot-sync would then attach
> > that BDS as the snapshot-file that wraps an existing BDS (without
> > needing to worry about options).
> > 
> >>
> >> What we'd then need is a QMP command for creating images. But as far as
> >> I know we'll need that anyway sooner or later...
> > 
> > Can't blockdev-add already be used for that (at least, for supported
> > file types)? If not, what would it take to get it there?
> 
> It would take a blockdev-create-image QMP command. :-)
> 
> blockdev-add only opens existing images, blockdev-create-image would
> then create these so they can be opened using blockdev-add.
> 
> Similar to blockdev-add, it would probably have a single parameter, but
> it'd be of a different type, called e.g. BlockdevCreateOptions, since it
> has to reflect the creation options instead of the runtime options for
> opening existing images. For instance, for qcow2 you could set the
> refcount-bits value, but not the L2 cache size.

Would be nice to have (especially because we would get a schema of the
create options), but not absolutely necessary for a blockdev-* style
snapshotting command. libvirt seems to cope just fine with calling
qemu-img before going to the QMP monitor.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 1/1] block: Allow passing BlockdevOptions to blockdev-snapshot-sync

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 2152 bytes --]

Am 31.08.2015 um 22:05 hat Eric Blake geschrieben:
> On 08/31/2015 01:53 PM, Max Reitz wrote:
> 
> > Design question: Would it make sense to instead add a "reference" mode
> > to blockdev-snapshot-sync where you can specify a BDS's node-name
> > instead of snapshot-file to use an existing BDS as the new top layer,
> > ideally an empty one?
> 
> Indeed - then blockdev-add can be used to create an unattached BDS (with
> all appropriate options), and blockdev-snapshot-sync would then attach
> that BDS as the snapshot-file that wraps an existing BDS (without
> needing to worry about options).

Yes, this is what we should do.

The existing blockdev-snapshot-sync should really have been called
something like drive-snapshot, it doesn't belong in the blockdev-*
family of commands which works only with existing nodes.

> >> +++ b/qapi/block-core.json
> >> @@ -697,11 +697,18 @@
> >>  #
> >>  # @mode: #optional whether and how QEMU should create a new image, default is
> >>  #        'absolute-paths'.
> >> +#
> >> +# @options: #optional options for the new device, with the following
> >> +#           restrictions for the fields: 'driver' must match the value
> >> +#           of @format,
> > 
> > As said above, I'd rather make specifying both @options and @format
> > exclusive.
> > 
> > Maybe there is even some QAPI magic to enforce that (and for
> > 'node-name', too), I don't know...
> 
> Not that I know of at the moment, but not to say we can't add some.  The
> closest we can get is with a flat union, but that requires a
> non-optional discriminator field.  Maybe we can tweak qapi to make the
> discriminator optional (with a default value).  Thankfully, it sounds
> like Markus' work on introspection would at least let management apps
> learn about a new 'options' argument.

Let's avoid such magic and instead add a new, clean blockdev-* style
command. Maybe call it simply blockdev-snapshot; the -sync part was
added because we knew it wouldn't be the final version of the command.
Now we don't have any bdrv_open() in it any more that could by
synchronous or asynchronous.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 1/1] block: Allow passing BlockdevOptions to blockdev-snapshot-sync

[Alberto Garcia]

On Tue 01 Sep 2015 01:31:11 PM CEST, Kevin Wolf <kwolf@redhat.com> wrote:

>> > Design question: Would it make sense to instead add a "reference"
>> > mode to blockdev-snapshot-sync where you can specify a BDS's
>> > node-name instead of snapshot-file to use an existing BDS as the
>> > new top layer, ideally an empty one?
>> 
>> Indeed - then blockdev-add can be used to create an unattached BDS
>> (with all appropriate options), and blockdev-snapshot-sync would then
>> attach that BDS as the snapshot-file that wraps an existing BDS
>> (without needing to worry about options).
>
> Yes, this is what we should do.

Sounds like a good idea, thanks for the feedback !

>> >> +# @options: #optional options for the new device, with the following
>> >> +#           restrictions for the fields: 'driver' must match the value
>> >> +#           of @format,
>> > 
>> > As said above, I'd rather make specifying both @options and @format
>> > exclusive.
>> > 
>> > Maybe there is even some QAPI magic to enforce that (and for
>> > 'node-name', too), I don't know...
>> 
>> Not that I know of at the moment, but not to say we can't add some.  The
>> closest we can get is with a flat union, but that requires a
>> non-optional discriminator field.  Maybe we can tweak qapi to make the
>> discriminator optional (with a default value).  Thankfully, it sounds
>> like Markus' work on introspection would at least let management apps
>> learn about a new 'options' argument.

This is not necessary if we go for the "reference" mode proposed by
Markus, since the "options" parameter would disappear.

> Let's avoid such magic and instead add a new, clean blockdev-* style
> command. Maybe call it simply blockdev-snapshot; the -sync part was
> added because we knew it wouldn't be the final version of the command.
> Now we don't have any bdrv_open() in it any more that could by
> synchronous or asynchronous.

Would you then prefer me to create a new command instead of extending
the existing one? What would be the benefit (other than a better name)?

Berto

Re: [Qemu-devel] [PATCH 1/1] block: Allow passing BlockdevOptions to blockdev-snapshot-sync

[Kevin Wolf]

Am 01.09.2015 um 16:22 hat Alberto Garcia geschrieben:
> On Tue 01 Sep 2015 01:31:11 PM CEST, Kevin Wolf <kwolf@redhat.com> wrote:
> 
> >> > Design question: Would it make sense to instead add a "reference"
> >> > mode to blockdev-snapshot-sync where you can specify a BDS's
> >> > node-name instead of snapshot-file to use an existing BDS as the
> >> > new top layer, ideally an empty one?
> >> 
> >> Indeed - then blockdev-add can be used to create an unattached BDS
> >> (with all appropriate options), and blockdev-snapshot-sync would then
> >> attach that BDS as the snapshot-file that wraps an existing BDS
> >> (without needing to worry about options).
> >
> > Yes, this is what we should do.
> 
> Sounds like a good idea, thanks for the feedback !
> 
> >> >> +# @options: #optional options for the new device, with the following
> >> >> +#           restrictions for the fields: 'driver' must match the value
> >> >> +#           of @format,
> >> > 
> >> > As said above, I'd rather make specifying both @options and @format
> >> > exclusive.
> >> > 
> >> > Maybe there is even some QAPI magic to enforce that (and for
> >> > 'node-name', too), I don't know...
> >> 
> >> Not that I know of at the moment, but not to say we can't add some.  The
> >> closest we can get is with a flat union, but that requires a
> >> non-optional discriminator field.  Maybe we can tweak qapi to make the
> >> discriminator optional (with a default value).  Thankfully, it sounds
> >> like Markus' work on introspection would at least let management apps
> >> learn about a new 'options' argument.
> 
> This is not necessary if we go for the "reference" mode proposed by
> Markus, since the "options" parameter would disappear.
> 
> > Let's avoid such magic and instead add a new, clean blockdev-* style
> > command. Maybe call it simply blockdev-snapshot; the -sync part was
> > added because we knew it wouldn't be the final version of the command.
> > Now we don't have any bdrv_open() in it any more that could by
> > synchronous or asynchronous.
> 
> Would you then prefer me to create a new command instead of extending
> the existing one? What would be the benefit (other than a better name)?

A clean interface. There is really little overlap with what we have:

{ 'struct': 'BlockdevSnapshot',
  'data': { '*device': 'str', '*node-name': 'str',
            'snapshot-file': 'str', '*snapshot-node-name': 'str',
            '*format': 'str', '*mode': 'NewImageMode' } }

When you add an existing node which has been created with blockdev-add
as a snapshot, you won't use snapshot-file, snapshot-node-name, format
and mode. We would either have to make all of them optional and actually
forbid them when a reference is given, or to ensure that they are
consistent with the already existing node. That we have both device and
node-name (and both marked as optional, while one of them is required) is
also not in line with our current practise.

If we went further that way, the schema wouldn't really be expressive
any more because there would be too many hidden rules of which
combinations are allowed and which aren't.

What you really need for the version with a reference is just:

{ 'struct': 'BlockdevSnapshot',
  'data': { 'device': 'str', 'snapshot': 'str' } }

Where both arguments refer to a node by node-name or backend name.

Kevin

Re: [Qemu-devel] [PATCH 1/1] block: Allow passing BlockdevOptions to blockdev-snapshot-sync

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 01.09.2015 um 16:22 hat Alberto Garcia geschrieben:
[...]
>> Would you then prefer me to create a new command instead of extending
>> the existing one? What would be the benefit (other than a better name)?
>
> A clean interface. There is really little overlap with what we have:
>
> { 'struct': 'BlockdevSnapshot',
>   'data': { '*device': 'str', '*node-name': 'str',
>             'snapshot-file': 'str', '*snapshot-node-name': 'str',
>             '*format': 'str', '*mode': 'NewImageMode' } }
>
> When you add an existing node which has been created with blockdev-add
> as a snapshot, you won't use snapshot-file, snapshot-node-name, format
> and mode. We would either have to make all of them optional and actually
> forbid them when a reference is given, or to ensure that they are
> consistent with the already existing node. That we have both device and
> node-name (and both marked as optional, while one of them is required) is
> also not in line with our current practise.
>
> If we went further that way, the schema wouldn't really be expressive
> any more because there would be too many hidden rules of which
> combinations are allowed and which aren't.

Yes, and let me elaborate a bit.

QMP permits evolving stuff compatibly.  We use this capability to keep
things simple and short when we add functionality: we extend existing
stuff instead of duplicating and modifying it.  Say, add detail to a
query result, or add an optional command parameter that modifies
behavior.

The question to ask is whether the single, extended interface is still
simpler than a new interface plus the unchanged old one.

I expect introspection to influence our understanding of "simple
interface" going forward.  Stuff visible in introspection is usually
simple.  Rules of use introspection can't show are often problematic.

[...]

Re: [Qemu-devel] [PATCH 1/1] block: Allow passing BlockdevOptions to blockdev-snapshot-sync

[Alberto Garcia]

On Tue 01 Sep 2015 04:40:02 PM CEST, Kevin Wolf wrote:

>> > Let's avoid such magic and instead add a new, clean blockdev-*
>> > style command. Maybe call it simply blockdev-snapshot; the -sync
>> > part was added because we knew it wouldn't be the final version of
>> > the command.  Now we don't have any bdrv_open() in it any more that
>> > could by synchronous or asynchronous.

Ok, I have a first working prototype of the new command using
references as suggested.

> { 'struct': 'BlockdevSnapshot',
>   'data': { '*device': 'str', '*node-name': 'str',
>             'snapshot-file': 'str', '*snapshot-node-name': 'str',
>             '*format': 'str', '*mode': 'NewImageMode' } }
  [...]
>
> What you really need for the version with a reference is just:
>
> { 'struct': 'BlockdevSnapshot',
>   'data': { 'device': 'str', 'snapshot': 'str' } }

And what do I do with the old BlockdevSnapshot? I guess it can just be
renamed to BlockdevSnapshotSync or something like that?

Berto

Re: [Qemu-devel] [PATCH 1/1] block: Allow passing BlockdevOptions to blockdev-snapshot-sync

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1451 bytes --]

On 09/02/2015 08:23 AM, Alberto Garcia wrote:
> On Tue 01 Sep 2015 04:40:02 PM CEST, Kevin Wolf wrote:
> 
>>>> Let's avoid such magic and instead add a new, clean blockdev-*
>>>> style command. Maybe call it simply blockdev-snapshot; the -sync
>>>> part was added because we knew it wouldn't be the final version of
>>>> the command.  Now we don't have any bdrv_open() in it any more that
>>>> could by synchronous or asynchronous.
> 
> Ok, I have a first working prototype of the new command using
> references as suggested.
> 
>> { 'struct': 'BlockdevSnapshot',
>>   'data': { '*device': 'str', '*node-name': 'str',
>>             'snapshot-file': 'str', '*snapshot-node-name': 'str',
>>             '*format': 'str', '*mode': 'NewImageMode' } }
>   [...]
>>
>> What you really need for the version with a reference is just:
>>
>> { 'struct': 'BlockdevSnapshot',
>>   'data': { 'device': 'str', 'snapshot': 'str' } }
> 
> And what do I do with the old BlockdevSnapshot? I guess it can just be
> renamed to BlockdevSnapshotSync or something like that?

Sure. And one of the nice things with the ongoing introspection work is
that qapi type names are NOT exposed as ABI (changing the name of a type
affects the generated C code, but does not affect what a QMP client
sends over the wire), so it is safe to do.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]