[1/2] commit: Add top-node/base-node options

Message ID 20180810162658.6562-2-kwolf@redhat.com
State New
Headers show
Series
  • commit: Add top-node/base-node options
Related show

Commit Message

Kevin Wolf Aug. 10, 2018, 4:26 p.m.
The block-commit QMP command required specifying the top and base nodes
of the commit jobs using the file name of that node. While this works
in simple cases (local files with absolute paths), the file names
generated for more complicated setups can be hard to predict.

This adds two new options top-node and base-node to the command, which
allow specifying node names instead. They are mutually exclusive with
the old options.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json | 24 ++++++++++++++++++------
 blockdev.c           | 32 ++++++++++++++++++++++++++++++--
 2 files changed, 48 insertions(+), 8 deletions(-)

Comments

Eric Blake Aug. 10, 2018, 5:33 p.m. | #1
On 08/10/2018 11:26 AM, Kevin Wolf wrote:
> The block-commit QMP command required specifying the top and base nodes
> of the commit jobs using the file name of that node. While this works
> in simple cases (local files with absolute paths), the file names
> generated for more complicated setups can be hard to predict.
> 
> This adds two new options top-node and base-node to the command, which
> allow specifying node names instead. They are mutually exclusive with
> the old options.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   qapi/block-core.json | 24 ++++++++++++++++++------
>   blockdev.c           | 32 ++++++++++++++++++++++++++++++--
>   2 files changed, 48 insertions(+), 8 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 5b9084a394..91dd075c84 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1455,12 +1455,23 @@
>   #
>   # @device:  the device name or node-name of a root node
>   #
> -# @base:   The file name of the backing image to write data into.
> -#                    If not specified, this is the deepest backing image.
> +# @base-node: The node name of the backing image to write data into.
> +#             If not specified, this is the deepest backing image.
> +#             (since: 2.10)

I'd word this as (since 3.1)...

>   #
> -# @top:    The file name of the backing image within the image chain,
> -#                    which contains the topmost data to be committed down. If
> -#                    not specified, this is the active layer.
> +# @base: Same as @base-node, except that it is a file name rather than a node
> +#        name. This must be the exact filename string that was used to open the
> +#        node; other strings, even if addressing the same file, are not
> +#        accepted (deprecated, use @base-node instead)

...and this as (since 2.10). When we finish the deprecation and remove 
@base, then we might consolidate the 'since' documentation at that time, 
but until then, I think listing the two separate releases gives users an 
idea of how far back they might have been using the deprecated code, and 
when the preferred form was introduced.

> +#
> +# @top-node: The node name of the backing image within the image chain
> +#            which contains the topmost data to be committed down. If
> +#            not specified, this is the active layer. (since: 2.10)
> +#
> +# @top: Same as @top-node, except that it is a file name rather than a node
> +#       name. This must be the exact filename string that was used to open the
> +#       node; other strings, even if addressing the same file, are not
> +#       accepted (deprecated, use @base-node instead)

Likewise.

Actually, do we NEED new arguments? Can we just make @base and @top 
accept either an exact file name OR a node name?  On the other hand, new 
arguments are introspectible, overloading the old argument to take two 
forms is not.  So that doesn't help :(

Or, here's an idea:

Keep the name @base and @top, but add a new '*by-node':'bool' parameter, 
defaulting to false for now, but perhaps with a deprecation warning that 
we'll switch the default to true in one release and delete the parameter 
altogether in an even later release. When false, @base and @top are 
filenames, as before; when true, @base and @top are node names instead. 
Introspectible, nicer names in the long run, and avoids having to detect 
a user providing two mutually-exclusive options at once.

> +++ b/blockdev.c
> @@ -3308,7 +3308,9 @@ out:
>   }
>   
>   void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
> +                      bool has_base_node, const char *base_node,
>                         bool has_base, const char *base,
> +                      bool has_top_node, const char *top_node,
>                         bool has_top, const char *top,
>                         bool has_backing_file, const char *backing_file,
>                         bool has_speed, int64_t speed,

Getting to be a long signature. Should we use 'boxed':true in the QAPI 
file to make this easier to write?  (Separate commit)
Kevin Wolf Aug. 13, 2018, 9:08 a.m. | #2
Am 10.08.2018 um 19:33 hat Eric Blake geschrieben:
> On 08/10/2018 11:26 AM, Kevin Wolf wrote:
> > The block-commit QMP command required specifying the top and base nodes
> > of the commit jobs using the file name of that node. While this works
> > in simple cases (local files with absolute paths), the file names
> > generated for more complicated setups can be hard to predict.
> > 
> > This adds two new options top-node and base-node to the command, which
> > allow specifying node names instead. They are mutually exclusive with
> > the old options.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   qapi/block-core.json | 24 ++++++++++++++++++------
> >   blockdev.c           | 32 ++++++++++++++++++++++++++++++--
> >   2 files changed, 48 insertions(+), 8 deletions(-)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 5b9084a394..91dd075c84 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -1455,12 +1455,23 @@
> >   #
> >   # @device:  the device name or node-name of a root node
> >   #
> > -# @base:   The file name of the backing image to write data into.
> > -#                    If not specified, this is the deepest backing image.
> > +# @base-node: The node name of the backing image to write data into.
> > +#             If not specified, this is the deepest backing image.
> > +#             (since: 2.10)
> 
> I'd word this as (since 3.1)...

Whoops. Apparently I didn't read the documentation change carefully
enough when resurrecting this patch from an old branch.

> >  #
> > -# @top:    The file name of the backing image within the image chain,
> > -#                    which contains the topmost data to be committed down. If
> > -#                    not specified, this is the active layer.
> > +# @base: Same as @base-node, except that it is a file name rather than a node
> > +#        name. This must be the exact filename string that was used to open the
> > +#        node; other strings, even if addressing the same file, are not
> > +#        accepted (deprecated, use @base-node instead)
> 
> ...and this as (since 2.10).

No, 2.10 is just completely wrong. @base exists since the command was
introduced, which is commit ed61fc10e8c or QEMU 1.3.

> When we finish the deprecation and remove @base, then we might
> consolidate the 'since' documentation at that time, but until then, I
> think listing the two separate releases gives users an idea of how far
> back they might have been using the deprecated code, and when the
> preferred form was introduced.

Yes, obviously.

> > +#
> > +# @top-node: The node name of the backing image within the image chain
> > +#            which contains the topmost data to be committed down. If
> > +#            not specified, this is the active layer. (since: 2.10)
> > +#
> > +# @top: Same as @top-node, except that it is a file name rather than a node
> > +#       name. This must be the exact filename string that was used to open the
> > +#       node; other strings, even if addressing the same file, are not
> > +#       accepted (deprecated, use @base-node instead)
> 
> Likewise.
> 
> Actually, do we NEED new arguments? Can we just make @base and @top accept
> either an exact file name OR a node name?

No, no, no, no, no!

You can't tell whether "foo" is a file name or a node name, and they
could both exist at the same time, so it would be ambiguous. We should
avoid mixing semantically different things in a single field whenever
it's possible.

The reason why node name and BlockBackend name can be used in the same
option is that they share a name space, i.e. if there is already a node
name "foo", trying to create a BlockBackend "foo" will fail, and vice
versa.

> On the other hand, new arguments are introspectible, overloading the
> old argument to take two forms is not.
> So that doesn't help :(

That, too, yes.

> Or, here's an idea:
> 
> Keep the name @base and @top, but add a new '*by-node':'bool' parameter,
> defaulting to false for now, but perhaps with a deprecation warning that
> we'll switch the default to true in one release and delete the parameter
> altogether in an even later release. When false, @base and @top are
> filenames, as before; when true, @base and @top are node names instead.
> Introspectible, nicer names in the long run, and avoids having to detect a
> user providing two mutually-exclusive options at once.

I don't like options that completely change the semantics of other
options, but maybe that's just personal preference.

Anyway, thinking about the long term for block-commit is useless, the
command is just broken (for example, the @device option doesn't make any
sense) and will have to be replaced. But libvirt needs something _now_
for the -blockdev support, so I decided to add this as a quick hack
before we get the proper replacement.

I think it makes more sense to create a new blockdev-commit (which
would be a name more in line with the other block job commands) and
deprecate the old block-commit command as a whole.

> > +++ b/blockdev.c
> > @@ -3308,7 +3308,9 @@ out:
> >   }
> >   void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
> > +                      bool has_base_node, const char *base_node,
> >                         bool has_base, const char *base,
> > +                      bool has_top_node, const char *top_node,
> >                         bool has_top, const char *top,
> >                         bool has_backing_file, const char *backing_file,
> >                         bool has_speed, int64_t speed,
> 
> Getting to be a long signature. Should we use 'boxed':true in the QAPI file
> to make this easier to write?  (Separate commit)

It's an option.

Has any progress been made on the plan to support defaults in QAPI, so
that we could get rid of the has_* parameters?

Kevin
Markus Armbruster Aug. 13, 2018, 9:35 a.m. | #3
Kevin Wolf <kwolf@redhat.com> writes:

> Am 10.08.2018 um 19:33 hat Eric Blake geschrieben:
>> On 08/10/2018 11:26 AM, Kevin Wolf wrote:
>> > The block-commit QMP command required specifying the top and base nodes
>> > of the commit jobs using the file name of that node. While this works
>> > in simple cases (local files with absolute paths), the file names
>> > generated for more complicated setups can be hard to predict.
>> > 
>> > This adds two new options top-node and base-node to the command, which
>> > allow specifying node names instead. They are mutually exclusive with
>> > the old options.
>> > 
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > ---
>> >   qapi/block-core.json | 24 ++++++++++++++++++------
>> >   blockdev.c           | 32 ++++++++++++++++++++++++++++++--
>> >   2 files changed, 48 insertions(+), 8 deletions(-)
>> > 
>> > diff --git a/qapi/block-core.json b/qapi/block-core.json
>> > index 5b9084a394..91dd075c84 100644
>> > --- a/qapi/block-core.json
>> > +++ b/qapi/block-core.json
[...]
>> Or, here's an idea:
>> 
>> Keep the name @base and @top, but add a new '*by-node':'bool' parameter,
>> defaulting to false for now, but perhaps with a deprecation warning that
>> we'll switch the default to true in one release and delete the parameter
>> altogether in an even later release. When false, @base and @top are
>> filenames, as before; when true, @base and @top are node names instead.
>> Introspectible, nicer names in the long run, and avoids having to detect a
>> user providing two mutually-exclusive options at once.
>
> I don't like options that completely change the semantics of other
> options, but maybe that's just personal preference.

I happen to share it.

> Anyway, thinking about the long term for block-commit is useless, the
> command is just broken (for example, the @device option doesn't make any
> sense) and will have to be replaced. But libvirt needs something _now_
> for the -blockdev support, so I decided to add this as a quick hack
> before we get the proper replacement.
>
> I think it makes more sense to create a new blockdev-commit (which
> would be a name more in line with the other block job commands) and
> deprecate the old block-commit command as a whole.
>
>> > +++ b/blockdev.c
>> > @@ -3308,7 +3308,9 @@ out:
>> >   }
>> >   void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
>> > +                      bool has_base_node, const char *base_node,
>> >                         bool has_base, const char *base,
>> > +                      bool has_top_node, const char *top_node,
>> >                         bool has_top, const char *top,
>> >                         bool has_backing_file, const char *backing_file,
>> >                         bool has_speed, int64_t speed,
>> 
>> Getting to be a long signature. Should we use 'boxed':true in the QAPI file
>> to make this easier to write?  (Separate commit)
>
> It's an option.
>
> Has any progress been made on the plan to support defaults in QAPI, so
> that we could get rid of the has_* parameters?

No.  It's one of those things that keep getting pushed out by more
important or urgent stuff.

I expect it to be straightforward, if tedious.
Eric Blake Aug. 13, 2018, 3:08 p.m. | #4
On 08/13/2018 04:35 AM, Markus Armbruster wrote:

>>> Or, here's an idea:
>>>
>>> Keep the name @base and @top, but add a new '*by-node':'bool' parameter,
>>> defaulting to false for now, but perhaps with a deprecation warning that
>>> we'll switch the default to true in one release and delete the parameter
>>> altogether in an even later release. When false, @base and @top are
>>> filenames, as before; when true, @base and @top are node names instead.
>>> Introspectible, nicer names in the long run, and avoids having to detect a
>>> user providing two mutually-exclusive options at once.
>>
>> I don't like options that completely change the semantics of other
>> options, but maybe that's just personal preference.
> 
> I happen to share it.

Okay, we'll ditch that idea as a non-starter.

> 
>> Anyway, thinking about the long term for block-commit is useless, the
>> command is just broken (for example, the @device option doesn't make any
>> sense) and will have to be replaced. But libvirt needs something _now_
>> for the -blockdev support, so I decided to add this as a quick hack
>> before we get the proper replacement.
>>
>> I think it makes more sense to create a new blockdev-commit (which
>> would be a name more in line with the other block job commands) and
>> deprecate the old block-commit command as a whole.

Okay, looks like a good plan for the long term, and thus a good 
rationale for the short-term choices. The commit message could call that 
out.


>> Has any progress been made on the plan to support defaults in QAPI, so
>> that we could get rid of the has_* parameters?
> 
> No.  It's one of those things that keep getting pushed out by more
> important or urgent stuff.
> 
> I expect it to be straightforward, if tedious.

In part, Marc-Andre's work to get conditional compilation in has gotten 
us closer, in that we can have 'name':{'type':'foo','if':'...'} instead 
of 'name':'type', since that dict for conditional compilation is also 
where we would stick in default values.
Max Reitz Aug. 13, 2018, 4:40 p.m. | #5
On 2018-08-10 18:26, Kevin Wolf wrote:
> The block-commit QMP command required specifying the top and base nodes
> of the commit jobs using the file name of that node. While this works
> in simple cases (local files with absolute paths), the file names
> generated for more complicated setups can be hard to predict.
> 
> This adds two new options top-node and base-node to the command, which
> allow specifying node names instead. They are mutually exclusive with
> the old options.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/block-core.json | 24 ++++++++++++++++++------
>  blockdev.c           | 32 ++++++++++++++++++++++++++++++--
>  2 files changed, 48 insertions(+), 8 deletions(-)

Looks good to me, but you made me a bit cautious with your talk of how
many pitfalls you've encountered on your way to do this change...

Max

Patch

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5b9084a394..91dd075c84 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1455,12 +1455,23 @@ 
 #
 # @device:  the device name or node-name of a root node
 #
-# @base:   The file name of the backing image to write data into.
-#                    If not specified, this is the deepest backing image.
+# @base-node: The node name of the backing image to write data into.
+#             If not specified, this is the deepest backing image.
+#             (since: 2.10)
 #
-# @top:    The file name of the backing image within the image chain,
-#                    which contains the topmost data to be committed down. If
-#                    not specified, this is the active layer.
+# @base: Same as @base-node, except that it is a file name rather than a node
+#        name. This must be the exact filename string that was used to open the
+#        node; other strings, even if addressing the same file, are not
+#        accepted (deprecated, use @base-node instead)
+#
+# @top-node: The node name of the backing image within the image chain
+#            which contains the topmost data to be committed down. If
+#            not specified, this is the active layer. (since: 2.10)
+#
+# @top: Same as @top-node, except that it is a file name rather than a node
+#       name. This must be the exact filename string that was used to open the
+#       node; other strings, even if addressing the same file, are not
+#       accepted (deprecated, use @base-node instead)
 #
 # @backing-file:  The backing file string to write into the overlay
 #                           image of 'top'.  If 'top' is the active layer,
@@ -1516,7 +1527,8 @@ 
 #
 ##
 { 'command': 'block-commit',
-  'data': { '*job-id': 'str', 'device': 'str', '*base': 'str', '*top': 'str',
+  'data': { '*job-id': 'str', 'device': 'str', '*base-node': 'str',
+            '*base': 'str', '*top-node': 'str', '*top': 'str',
             '*backing-file': 'str', '*speed': 'int',
             '*filter-node-name': 'str' } }
 
diff --git a/blockdev.c b/blockdev.c
index dcf8c8d2ab..064c8fb3f5 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3308,7 +3308,9 @@  out:
 }
 
 void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
+                      bool has_base_node, const char *base_node,
                       bool has_base, const char *base,
+                      bool has_top_node, const char *top_node,
                       bool has_top, const char *top,
                       bool has_backing_file, const char *backing_file,
                       bool has_speed, int64_t speed,
@@ -3360,7 +3362,20 @@  void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
     /* default top_bs is the active layer */
     top_bs = bs;
 
-    if (has_top && top) {
+    if (has_top_node && has_top) {
+        error_setg(errp, "'top-node' and 'top' are mutually exclusive");
+        goto out;
+    } else if (has_top_node) {
+        top_bs = bdrv_lookup_bs(NULL, top_node, errp);
+        if (top_bs == NULL) {
+            goto out;
+        }
+        if (!bdrv_chain_contains(bs, top_bs)) {
+            error_setg(errp, "'%s' is not in this backing file chain",
+                       top_node);
+            goto out;
+        }
+    } else if (has_top && top) {
         if (strcmp(bs->filename, top) != 0) {
             top_bs = bdrv_find_backing_image(bs, top);
         }
@@ -3373,7 +3388,20 @@  void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
 
     assert(bdrv_get_aio_context(top_bs) == aio_context);
 
-    if (has_base && base) {
+    if (has_base_node && has_base) {
+        error_setg(errp, "'base-node' and 'base' are mutually exclusive");
+        goto out;
+    } else if (has_base_node) {
+        base_bs = bdrv_lookup_bs(NULL, base_node, errp);
+        if (base_bs == NULL) {
+            goto out;
+        }
+        if (!bdrv_chain_contains(top_bs, base_bs)) {
+            error_setg(errp, "'%s' is not in this backing file chain",
+                       base_node);
+            goto out;
+        }
+    } else if (has_base && base) {
         base_bs = bdrv_find_backing_image(top_bs, base);
     } else {
         base_bs = bdrv_find_base(top_bs);