[Qemu-devel] [PATCH 0/5] block: Modify block-commit to use node-names

[Qemu-devel] [PATCH 0/5] block: Modify block-commit to use node-names

[Jeff Cody]

Using node-names instead of filenames for block job operations
over QMP is a superior method of identifying the block driver
images to operate on, as it removes all pathname ambiguity.

This series is the conversion of block-commit to allow use of
node-names.  Also, it allows the user to specify the string for
the backing_file name to use in the overlay image.

So that node-names can be used as desired for all block job
operations, this series also auto-generates node-names for every
BDS.  User-specified node-names will override any autogenerated
node-names.

Subsequent patches will convert the remaining block operations
(stream, backup, mirror)

These patches can also be seen at: 
https://github.com/codyprime/qemu-kvm-jtc.git, tag block-commit-node-v1a

Jeff Cody (5):
  block: Auto-generate node_names for each BDS entry
  block: add helper function to determine if a BDS is in a chain
  block: make 'top' argument to block-commit optional
  block: Accept node-name arguments for block-commit
  block: extend block-commit to accept a string for the backing file

 block.c                   | 33 ++++++++++++++++++++++++++++---
 block/commit.c            |  9 ++++++---
 blockdev.c                | 46 +++++++++++++++++++++++++++++++++++++++----
 include/block/block.h     |  4 +++-
 include/block/block_int.h |  3 ++-
 qapi-schema.json          | 50 ++++++++++++++++++++++++++++++++++++++---------
 qmp-commands.hx           | 40 +++++++++++++++++++++++++++++++------
 tests/qemu-iotests/040    | 28 ++++++++++++++++----------
 8 files changed, 176 insertions(+), 37 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry

[Jeff Cody]

Currently, node_name is only filled in when done so explicitly by the
user.  If no node_name is specified, then the node name field is not
populated.

If node_names are automatically generated when not specified, that means
that all block job operations can be done by reference to the unique
node_name field.  This eliminates ambiguity in filename pathing
(relative filenames, or file descriptors, symlinks, mounts, etc..) that
qemu currently needs to deal with.

If a node name is specified, then it will not be automatically
generated for that BDS entry.

If it is automatically generated, it will be prefaced with "__qemu##",
followed by 8 characters of a unique number, followed by 8 random
ASCII characters in the range of 'A-Z'.  Some sample generated node-name
strings:
    __qemu##00000000IAIYNXXR
    __qemu##00000002METXTRBQ
    __qemu##00000001FMBORDWG

The prefix is to aid in identifying it as a qemu-generated name, the
numeric portion is to guarantee uniqueness in a given qemu session, and
the random characters are to further avoid any accidental collisions
with user-specified node-names.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index c90c71a..81945d3 100644
--- a/block.c
+++ b/block.c
@@ -838,12 +838,26 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
     return open_flags;
 }
 
+#define GEN_NODE_NAME_PREFIX    "__qemu##"
+#define GEN_NODE_NAME_MAX_LEN   (sizeof(GEN_NODE_NAME_PREFIX) + 8 + 8)
 static void bdrv_assign_node_name(BlockDriverState *bs,
                                   const char *node_name,
                                   Error **errp)
 {
+    char gen_node_name[GEN_NODE_NAME_MAX_LEN];
+    static uint32_t counter; /* simple counter to guarantee uniqueness */
+
+    /* if node_name is NULL, auto-generate a node name */
     if (!node_name) {
-        return;
+        int len;
+        snprintf(gen_node_name, GEN_NODE_NAME_MAX_LEN,
+                 "%s%08x", GEN_NODE_NAME_PREFIX, counter++);
+        len = strlen(gen_node_name);
+        while (len < GEN_NODE_NAME_MAX_LEN - 1) {
+            gen_node_name[len++] = g_random_int_range('A', 'Z');
+        }
+        gen_node_name[GEN_NODE_NAME_MAX_LEN - 1] = '\0';
+        node_name = gen_node_name;
     }
 
     /* empty string node name is invalid */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry

[Benoît Canet]

The Wednesday 14 May 2014 à 23:20:15 (-0400), Jeff Cody wrote :
> Currently, node_name is only filled in when done so explicitly by the
> user.  If no node_name is specified, then the node name field is not
> populated.
> 
> If node_names are automatically generated when not specified, that means
> that all block job operations can be done by reference to the unique
> node_name field.  This eliminates ambiguity in filename pathing
> (relative filenames, or file descriptors, symlinks, mounts, etc..) that
> qemu currently needs to deal with.
> 
> If a node name is specified, then it will not be automatically
> generated for that BDS entry.
> 
> If it is automatically generated, it will be prefaced with "__qemu##",
> followed by 8 characters of a unique number, followed by 8 random
> ASCII characters in the range of 'A-Z'.  Some sample generated node-name
> strings:
>     __qemu##00000000IAIYNXXR
>     __qemu##00000002METXTRBQ
>     __qemu##00000001FMBORDWG
> 
> The prefix is to aid in identifying it as a qemu-generated name, the
> numeric portion is to guarantee uniqueness in a given qemu session, and
> the random characters are to further avoid any accidental collisions
> with user-specified node-names.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index c90c71a..81945d3 100644
> --- a/block.c
> +++ b/block.c
> @@ -838,12 +838,26 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
>      return open_flags;
>  }
>  
> +#define GEN_NODE_NAME_PREFIX    "__qemu##"
> +#define GEN_NODE_NAME_MAX_LEN   (sizeof(GEN_NODE_NAME_PREFIX) + 8 + 8)
>  static void bdrv_assign_node_name(BlockDriverState *bs,
>                                    const char *node_name,
>                                    Error **errp)
>  {
> +    char gen_node_name[GEN_NODE_NAME_MAX_LEN];

The room for the '\0' string termination seems to be missing:

    char gen_node_name[GEN_NODE_NAME_MAX_LEN + 1];

> +    static uint32_t counter; /* simple counter to guarantee uniqueness */
> +
> +    /* if node_name is NULL, auto-generate a node name */
>      if (!node_name) {
> -        return;
> +        int len;
> +        snprintf(gen_node_name, GEN_NODE_NAME_MAX_LEN,
> +                 "%s%08x", GEN_NODE_NAME_PREFIX, counter++);
> +        len = strlen(gen_node_name);
> +        while (len < GEN_NODE_NAME_MAX_LEN - 1) {
> +            gen_node_name[len++] = g_random_int_range('A', 'Z');
> +        }

Is this code generating only 7 random chars instead of 8 ?

> +        gen_node_name[GEN_NODE_NAME_MAX_LEN - 1] = '\0';

Could be:
        gen_node_name[GEN_NODE_NAME_MAX_LEN] = '\0';
if the array is properly declared.

> +        node_name = gen_node_name;
>      }
>  
>      /* empty string node name is invalid */
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry

[Jeff Cody]

On Thu, May 15, 2014 at 01:58:59PM +0200, Benoît Canet wrote:
> The Wednesday 14 May 2014 à 23:20:15 (-0400), Jeff Cody wrote :
> > Currently, node_name is only filled in when done so explicitly by the
> > user.  If no node_name is specified, then the node name field is not
> > populated.
> > 
> > If node_names are automatically generated when not specified, that means
> > that all block job operations can be done by reference to the unique
> > node_name field.  This eliminates ambiguity in filename pathing
> > (relative filenames, or file descriptors, symlinks, mounts, etc..) that
> > qemu currently needs to deal with.
> > 
> > If a node name is specified, then it will not be automatically
> > generated for that BDS entry.
> > 
> > If it is automatically generated, it will be prefaced with "__qemu##",
> > followed by 8 characters of a unique number, followed by 8 random
> > ASCII characters in the range of 'A-Z'.  Some sample generated node-name
> > strings:
> >     __qemu##00000000IAIYNXXR
> >     __qemu##00000002METXTRBQ
> >     __qemu##00000001FMBORDWG
> > 
> > The prefix is to aid in identifying it as a qemu-generated name, the
> > numeric portion is to guarantee uniqueness in a given qemu session, and
> > the random characters are to further avoid any accidental collisions
> > with user-specified node-names.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block.c b/block.c
> > index c90c71a..81945d3 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -838,12 +838,26 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
> >      return open_flags;
> >  }
> >  
> > +#define GEN_NODE_NAME_PREFIX    "__qemu##"
> > +#define GEN_NODE_NAME_MAX_LEN   (sizeof(GEN_NODE_NAME_PREFIX) + 8 + 8)
> >  static void bdrv_assign_node_name(BlockDriverState *bs,
> >                                    const char *node_name,
> >                                    Error **errp)
> >  {
> > +    char gen_node_name[GEN_NODE_NAME_MAX_LEN];
> 
> The room for the '\0' string termination seems to be missing:
> 
>     char gen_node_name[GEN_NODE_NAME_MAX_LEN + 1];
>

The array includes room for it, note the use of 'sizeof()':
    #define GEN_NODE_NAME_MAX_LEN   (sizeof(GEN_NODE_NAME_PREFIX) + 8 + 8)

sizeof() includes the '\0' in the length, while strlen() does not;
e.g.:
    sizeof("four") = 5
    strlen("four") = 4

> > +    static uint32_t counter; /* simple counter to guarantee uniqueness */
> > +
> > +    /* if node_name is NULL, auto-generate a node name */
> >      if (!node_name) {
> > -        return;
> > +        int len;
> > +        snprintf(gen_node_name, GEN_NODE_NAME_MAX_LEN,
> > +                 "%s%08x", GEN_NODE_NAME_PREFIX, counter++);
> > +        len = strlen(gen_node_name);
> > +        while (len < GEN_NODE_NAME_MAX_LEN - 1) {
> > +            gen_node_name[len++] = g_random_int_range('A', 'Z');
> > +        }
> 
> Is this code generating only 7 random chars instead of 8 ?
> 

It generates 8 random characters (the sample node-name strings in the
commit message were pulled straight from the QMP command
'query-named-block-nodes')

> > +        gen_node_name[GEN_NODE_NAME_MAX_LEN - 1] = '\0';
> 
> Could be:
>         gen_node_name[GEN_NODE_NAME_MAX_LEN] = '\0';
> if the array is properly declared.
>

That would go over the array bounds by 1.

> > +        node_name = gen_node_name;
> >      }
> >  
> >      /* empty string node name is invalid */
> > -- 
> > 1.8.3.1
> > 

Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry

[Benoît Canet]

The Thursday 15 May 2014 à 08:06:14 (-0400), Jeff Cody wrote :
> On Thu, May 15, 2014 at 01:58:59PM +0200, Benoît Canet wrote:
> > The Wednesday 14 May 2014 à 23:20:15 (-0400), Jeff Cody wrote :
> > > Currently, node_name is only filled in when done so explicitly by the
> > > user.  If no node_name is specified, then the node name field is not
> > > populated.
> > > 
> > > If node_names are automatically generated when not specified, that means
> > > that all block job operations can be done by reference to the unique
> > > node_name field.  This eliminates ambiguity in filename pathing
> > > (relative filenames, or file descriptors, symlinks, mounts, etc..) that
> > > qemu currently needs to deal with.
> > > 
> > > If a node name is specified, then it will not be automatically
> > > generated for that BDS entry.
> > > 
> > > If it is automatically generated, it will be prefaced with "__qemu##",
> > > followed by 8 characters of a unique number, followed by 8 random
> > > ASCII characters in the range of 'A-Z'.  Some sample generated node-name
> > > strings:
> > >     __qemu##00000000IAIYNXXR
> > >     __qemu##00000002METXTRBQ
> > >     __qemu##00000001FMBORDWG
> > > 
> > > The prefix is to aid in identifying it as a qemu-generated name, the
> > > numeric portion is to guarantee uniqueness in a given qemu session, and
> > > the random characters are to further avoid any accidental collisions
> > > with user-specified node-names.
> > > 
> > > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > > ---
> > >  block.c | 16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/block.c b/block.c
> > > index c90c71a..81945d3 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -838,12 +838,26 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
> > >      return open_flags;
> > >  }
> > >  
> > > +#define GEN_NODE_NAME_PREFIX    "__qemu##"
> > > +#define GEN_NODE_NAME_MAX_LEN   (sizeof(GEN_NODE_NAME_PREFIX) + 8 + 8)
> > >  static void bdrv_assign_node_name(BlockDriverState *bs,
> > >                                    const char *node_name,
> > >                                    Error **errp)
> > >  {
> > > +    char gen_node_name[GEN_NODE_NAME_MAX_LEN];
> > 
> > The room for the '\0' string termination seems to be missing:
> > 
> >     char gen_node_name[GEN_NODE_NAME_MAX_LEN + 1];
> >
> 
> The array includes room for it, note the use of 'sizeof()':
>     #define GEN_NODE_NAME_MAX_LEN   (sizeof(GEN_NODE_NAME_PREFIX) + 8 + 8)
> 
> sizeof() includes the '\0' in the length, while strlen() does not;
> e.g.:
>     sizeof("four") = 5
>     strlen("four") = 4
> 
> > > +    static uint32_t counter; /* simple counter to guarantee uniqueness */
> > > +
> > > +    /* if node_name is NULL, auto-generate a node name */
> > >      if (!node_name) {
> > > -        return;
> > > +        int len;
> > > +        snprintf(gen_node_name, GEN_NODE_NAME_MAX_LEN,
> > > +                 "%s%08x", GEN_NODE_NAME_PREFIX, counter++);
> > > +        len = strlen(gen_node_name);
> > > +        while (len < GEN_NODE_NAME_MAX_LEN - 1) {
> > > +            gen_node_name[len++] = g_random_int_range('A', 'Z');
> > > +        }
> > 
> > Is this code generating only 7 random chars instead of 8 ?
> > 
> 
> It generates 8 random characters (the sample node-name strings in the
> commit message were pulled straight from the QMP command
> 'query-named-block-nodes')
> 
> > > +        gen_node_name[GEN_NODE_NAME_MAX_LEN - 1] = '\0';
> > 
> > Could be:
> >         gen_node_name[GEN_NODE_NAME_MAX_LEN] = '\0';
> > if the array is properly declared.
> >
> 
> That would go over the array bounds by 1.

Yes I missed the use of sizeof().
I am happy to have learnt that.
Sorry for the noise.

> 
> > > +        node_name = gen_node_name;
> > >      }
> > >  
> > >      /* empty string node name is invalid */
> > > -- 
> > > 1.8.3.1
> > > 

Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry

[Jeff Cody]

On Thu, May 15, 2014 at 02:32:50PM +0200, Benoît Canet wrote:
> The Thursday 15 May 2014 à 08:06:14 (-0400), Jeff Cody wrote :
> > On Thu, May 15, 2014 at 01:58:59PM +0200, Benoît Canet wrote:
> > > The Wednesday 14 May 2014 à 23:20:15 (-0400), Jeff Cody wrote :
> > > > Currently, node_name is only filled in when done so explicitly by the
> > > > user.  If no node_name is specified, then the node name field is not
> > > > populated.
> > > > 
> > > > If node_names are automatically generated when not specified, that means
> > > > that all block job operations can be done by reference to the unique
> > > > node_name field.  This eliminates ambiguity in filename pathing
> > > > (relative filenames, or file descriptors, symlinks, mounts, etc..) that
> > > > qemu currently needs to deal with.
> > > > 
> > > > If a node name is specified, then it will not be automatically
> > > > generated for that BDS entry.
> > > > 
> > > > If it is automatically generated, it will be prefaced with "__qemu##",
> > > > followed by 8 characters of a unique number, followed by 8 random
> > > > ASCII characters in the range of 'A-Z'.  Some sample generated node-name
> > > > strings:
> > > >     __qemu##00000000IAIYNXXR
> > > >     __qemu##00000002METXTRBQ
> > > >     __qemu##00000001FMBORDWG
> > > > 
> > > > The prefix is to aid in identifying it as a qemu-generated name, the
> > > > numeric portion is to guarantee uniqueness in a given qemu session, and
> > > > the random characters are to further avoid any accidental collisions
> > > > with user-specified node-names.
> > > > 
> > > > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > > > ---
> > > >  block.c | 16 +++++++++++++++-
> > > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/block.c b/block.c
> > > > index c90c71a..81945d3 100644
> > > > --- a/block.c
> > > > +++ b/block.c
> > > > @@ -838,12 +838,26 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
> > > >      return open_flags;
> > > >  }
> > > >  
> > > > +#define GEN_NODE_NAME_PREFIX    "__qemu##"
> > > > +#define GEN_NODE_NAME_MAX_LEN   (sizeof(GEN_NODE_NAME_PREFIX) + 8 + 8)
> > > >  static void bdrv_assign_node_name(BlockDriverState *bs,
> > > >                                    const char *node_name,
> > > >                                    Error **errp)
> > > >  {
> > > > +    char gen_node_name[GEN_NODE_NAME_MAX_LEN];
> > > 
> > > The room for the '\0' string termination seems to be missing:
> > > 
> > >     char gen_node_name[GEN_NODE_NAME_MAX_LEN + 1];
> > >
> > 
> > The array includes room for it, note the use of 'sizeof()':
> >     #define GEN_NODE_NAME_MAX_LEN   (sizeof(GEN_NODE_NAME_PREFIX) + 8 + 8)
> > 
> > sizeof() includes the '\0' in the length, while strlen() does not;
> > e.g.:
> >     sizeof("four") = 5
> >     strlen("four") = 4
> > 
> > > > +    static uint32_t counter; /* simple counter to guarantee uniqueness */
> > > > +
> > > > +    /* if node_name is NULL, auto-generate a node name */
> > > >      if (!node_name) {
> > > > -        return;
> > > > +        int len;
> > > > +        snprintf(gen_node_name, GEN_NODE_NAME_MAX_LEN,
> > > > +                 "%s%08x", GEN_NODE_NAME_PREFIX, counter++);
> > > > +        len = strlen(gen_node_name);
> > > > +        while (len < GEN_NODE_NAME_MAX_LEN - 1) {
> > > > +            gen_node_name[len++] = g_random_int_range('A', 'Z');
> > > > +        }
> > > 
> > > Is this code generating only 7 random chars instead of 8 ?
> > > 
> > 
> > It generates 8 random characters (the sample node-name strings in the
> > commit message were pulled straight from the QMP command
> > 'query-named-block-nodes')
> > 
> > > > +        gen_node_name[GEN_NODE_NAME_MAX_LEN - 1] = '\0';
> > > 
> > > Could be:
> > >         gen_node_name[GEN_NODE_NAME_MAX_LEN] = '\0';
> > > if the array is properly declared.
> > >
> > 
> > That would go over the array bounds by 1.
> 
> Yes I missed the use of sizeof().
> I am happy to have learnt that.
> Sorry for the noise.
>

It wasn't noise :)  Thanks for the reviews.

> > 
> > > > +        node_name = gen_node_name;
> > > >      }
> > > >  
> > > >      /* empty string node name is invalid */
> > > > -- 
> > > > 1.8.3.1
> > > > 

Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry

[Eric Blake]

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

On 05/14/2014 09:20 PM, Jeff Cody wrote:
> Currently, node_name is only filled in when done so explicitly by the
> user.  If no node_name is specified, then the node name field is not
> populated.
> 
> If node_names are automatically generated when not specified, that means
> that all block job operations can be done by reference to the unique
> node_name field.  This eliminates ambiguity in filename pathing

"pathing" sounds weird; a non-native speaker may get confused since it
is not a real word. Maybe:

This eliminates ambiguity in resolving filenames

> (relative filenames, or file descriptors, symlinks, mounts, etc..) that
> qemu currently needs to deal with.
> 
> If a node name is specified, then it will not be automatically
> generated for that BDS entry.
> 
> If it is automatically generated, it will be prefaced with "__qemu##",
> followed by 8 characters of a unique number, followed by 8 random
> ASCII characters in the range of 'A-Z'.  Some sample generated node-name
> strings:
>     __qemu##00000000IAIYNXXR
>     __qemu##00000002METXTRBQ
>     __qemu##00000001FMBORDWG
> 
> The prefix is to aid in identifying it as a qemu-generated name, the
> numeric portion is to guarantee uniqueness in a given qemu session, and
> the random characters are to further avoid any accidental collisions
> with user-specified node-names.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 

>  {
> +    char gen_node_name[GEN_NODE_NAME_MAX_LEN];
> +    static uint32_t counter; /* simple counter to guarantee uniqueness */
> +
> +    /* if node_name is NULL, auto-generate a node name */
>      if (!node_name) {
> -        return;
> +        int len;
> +        snprintf(gen_node_name, GEN_NODE_NAME_MAX_LEN,
> +                 "%s%08x", GEN_NODE_NAME_PREFIX, counter++);
> +        len = strlen(gen_node_name);

Why not just len = snprintf(), and avoid the strlen()?

But that micro-optimization is not on the hot path, so:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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/5] block: Auto-generate node_names for each BDS entry

[Eric Blake]

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

On 05/14/2014 09:20 PM, Jeff Cody wrote:
> Currently, node_name is only filled in when done so explicitly by the
> user.  If no node_name is specified, then the node name field is not
> populated.
> 
> If node_names are automatically generated when not specified, that means
> that all block job operations can be done by reference to the unique
> node_name field.  This eliminates ambiguity in filename pathing
> (relative filenames, or file descriptors, symlinks, mounts, etc..) that
> qemu currently needs to deal with.
> 
> If a node name is specified, then it will not be automatically
> generated for that BDS entry.
> 
> If it is automatically generated, it will be prefaced with "__qemu##",
> followed by 8 characters of a unique number, followed by 8 random
> ASCII characters in the range of 'A-Z'.  Some sample generated node-name
> strings:
>     __qemu##00000000IAIYNXXR
>     __qemu##00000002METXTRBQ
>     __qemu##00000001FMBORDWG
> 
> The prefix is to aid in identifying it as a qemu-generated name, the
> numeric portion is to guarantee uniqueness in a given qemu session, and
> the random characters are to further avoid any accidental collisions
> with user-specified node-names.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)

This patch feels incomplete.  We probably also ought to modify
qapi-schema.json to make BlockDeviceInfo's output of 'node-name' by
unconditional (as an output-only struct, changing from optional to
mandatory is a safe change for back-compat API considerations); which
implies further modifying code to get rid of has_node_name arguments in
all places that generate BlockDeviceInfo details.

See also my thoughts on patch 5/5 - can this patch be rebased to be LAST
in the series, rather than first, so that it serves as a reliable
witness that everything else related to block operations on node-names
is complete?

-- 
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/5] block: Auto-generate node_names for each BDS entry

[Jeff Cody]

On Thu, May 15, 2014 at 09:59:01AM -0600, Eric Blake wrote:
> On 05/14/2014 09:20 PM, Jeff Cody wrote:
> > Currently, node_name is only filled in when done so explicitly by the
> > user.  If no node_name is specified, then the node name field is not
> > populated.
> > 
> > If node_names are automatically generated when not specified, that means
> > that all block job operations can be done by reference to the unique
> > node_name field.  This eliminates ambiguity in filename pathing
> > (relative filenames, or file descriptors, symlinks, mounts, etc..) that
> > qemu currently needs to deal with.
> > 
> > If a node name is specified, then it will not be automatically
> > generated for that BDS entry.
> > 
> > If it is automatically generated, it will be prefaced with "__qemu##",
> > followed by 8 characters of a unique number, followed by 8 random
> > ASCII characters in the range of 'A-Z'.  Some sample generated node-name
> > strings:
> >     __qemu##00000000IAIYNXXR
> >     __qemu##00000002METXTRBQ
> >     __qemu##00000001FMBORDWG
> > 
> > The prefix is to aid in identifying it as a qemu-generated name, the
> > numeric portion is to guarantee uniqueness in a given qemu session, and
> > the random characters are to further avoid any accidental collisions
> > with user-specified node-names.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> This patch feels incomplete.  We probably also ought to modify
> qapi-schema.json to make BlockDeviceInfo's output of 'node-name' by
> unconditional (as an output-only struct, changing from optional to
> mandatory is a safe change for back-compat API considerations); which
> implies further modifying code to get rid of has_node_name arguments in
> all places that generate BlockDeviceInfo details.

I think it should be a separate patch (but still in this series).
Strictly speaking, this patch doesn't force the argument to be
mandatory, the value just happens to always be populated now.

> See also my thoughts on patch 5/5 - can this patch be rebased to be LAST
> in the series, rather than first, so that it serves as a reliable
> witness that everything else related to block operations on node-names
> is complete?
>
How about this becomes the penultimate patch, and the patch to make
BlockDeviceInfo's node-name field to be unconditional becomes the last
patch?

The only thing I don't like about moving this further back in the
patch series is it makes the earlier patches untestable; I can't
easily test the usage of the node-names for various intermediate BDS
because they don't have node-names set.  So that means I'll just need
to rebase the patches prior to sending.  So let me revise my above
statement - how about this patch stays at the beginning, which makes
development / testing easier, with the patch that modifies
BlockDeviceInfo as the final (not including tests) patch?

Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry

[Eric Blake]

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

On 05/15/2014 12:41 PM, Jeff Cody wrote:

>>> The prefix is to aid in identifying it as a qemu-generated name, the
>>> numeric portion is to guarantee uniqueness in a given qemu session, and
>>> the random characters are to further avoid any accidental collisions
>>> with user-specified node-names.
>>>
>>> Signed-off-by: Jeff Cody <jcody@redhat.com>
>>> ---
>>>  block.c | 16 +++++++++++++++-
>>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> This patch feels incomplete.  We probably also ought to modify
>> qapi-schema.json to make BlockDeviceInfo's output of 'node-name' by
>> unconditional (as an output-only struct, changing from optional to
>> mandatory is a safe change for back-compat API considerations); which
>> implies further modifying code to get rid of has_node_name arguments in
>> all places that generate BlockDeviceInfo details.
> 
> I think it should be a separate patch (but still in this series).
> Strictly speaking, this patch doesn't force the argument to be
> mandatory, the value just happens to always be populated now.

True, keeping it as two patches is cleaner.

> 
>> See also my thoughts on patch 5/5 - can this patch be rebased to be LAST
>> in the series, rather than first, so that it serves as a reliable
>> witness that everything else related to block operations on node-names
>> is complete?
>>
> How about this becomes the penultimate patch, and the patch to make
> BlockDeviceInfo's node-name field to be unconditional becomes the last
> patch?

Or, if we go the route of adding a new command for setting the backing
name in isolation, then _that_ patch should be last, at which point
leaving this patch early in the series no longer hurts, because it is no
longer the witness that libvirt would be relying on.

> 
> The only thing I don't like about moving this further back in the
> patch series is it makes the earlier patches untestable; I can't
> easily test the usage of the node-names for various intermediate BDS
> because they don't have node-names set.  So that means I'll just need
> to rebase the patches prior to sending.  So let me revise my above
> statement - how about this patch stays at the beginning, which makes
> development / testing easier, with the patch that modifies
> BlockDeviceInfo as the final (not including tests) patch?

Yes, keeping this patch first sounds reasonable after all.  The patch
modifying BlockDeviceInfo isn't introspectible (there's nothing libvirt
can probe that says whether the QMP code switched from optional to
mandatory - all libvirt can probe is whether a name gets generated - and
that is THIS patch, not the BlockDeviceInfo patch).  But adding a new
command is easier to use than probing whether a name was generated.

> 
> 

-- 
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/5] block: Auto-generate node_names for each BDS entry

[Kevin Wolf]

Am 15.05.2014 um 20:41 hat Jeff Cody geschrieben:
> The only thing I don't like about moving this further back in the
> patch series is it makes the earlier patches untestable; I can't
> easily test the usage of the node-names for various intermediate BDS
> because they don't have node-names set.  So that means I'll just need
> to rebase the patches prior to sending.

I don't quite follow. Can't you always manually assign node-names? This
is how libvirt is supposed to use the interface.

I'm not totally sure whether automatically generated node-names are
a good idea, but I can see how they are useful with human monitor users
which may not specify a node-name everywhere (I've used device_add
without an ID often enough, only to find that I can't remove the device
any more). We should just make sure that they are really only used by
human users.

Kevin

Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry

[Jeff Cody]

On Fri, May 16, 2014 at 11:39:27AM +0200, Kevin Wolf wrote:
> Am 15.05.2014 um 20:41 hat Jeff Cody geschrieben:
> > The only thing I don't like about moving this further back in the
> > patch series is it makes the earlier patches untestable; I can't
> > easily test the usage of the node-names for various intermediate BDS
> > because they don't have node-names set.  So that means I'll just need
> > to rebase the patches prior to sending.
> 
> I don't quite follow. Can't you always manually assign node-names? This
> is how libvirt is supposed to use the interface.
>

How does libvirt assign node-names to all the backing images in a
qcow2 chain, for example?


> I'm not totally sure whether automatically generated node-names are
> a good idea, but I can see how they are useful with human monitor users
> which may not specify a node-name everywhere (I've used device_add
> without an ID often enough, only to find that I can't remove the device
> any more). We should just make sure that they are really only used by
> human users.
>

I don't understand.  What would be the downsides of having an
automatic guaranteed unique id assigned to each BDS?  And why
restrict that to human users only?

If you are worried about node-names potentially being undesired by the
user / management layer for some reason, how about this: we add a
drive option, to either enable or disable automatic node-name
generation for a particular drive?

Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry

[Eric Blake]

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

On 05/16/2014 05:35 AM, Jeff Cody wrote:

> 
> How does libvirt assign node-names to all the backing images in a
> qcow2 chain, for example?

We have the ability to do both command-line and QMP hotplug addition of
drives where you can set backing.file.node-name (or is it
backing.node-name?) for any element in a backing chain; libvirt just
needs to be taught to use it.

> 
> 
>> I'm not totally sure whether automatically generated node-names are
>> a good idea, but I can see how they are useful with human monitor users
>> which may not specify a node-name everywhere (I've used device_add
>> without an ID often enough, only to find that I can't remove the device
>> any more). We should just make sure that they are really only used by
>> human users.
>>
> 
> I don't understand.  What would be the downsides of having an
> automatic guaranteed unique id assigned to each BDS?  And why
> restrict that to human users only?

I'm not seeing downsides.

> 
> If you are worried about node-names potentially being undesired by the
> user / management layer for some reason, how about this: we add a
> drive option, to either enable or disable automatic node-name
> generation for a particular drive?

No, this is silly.  If it defaults to off, then libvirt has to be taught
to enable it - but if you are going to modify libvirt, you might as well
modify it to set node names itself rather than to enable the option for
automatic node names.  If it defaults to on, no one has an incentive to
disable it.

-- 
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/5] block: Auto-generate node_names for each BDS entry

[Kevin Wolf]

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

Am 16.05.2014 um 14:47 hat Eric Blake geschrieben:
> On 05/16/2014 05:35 AM, Jeff Cody wrote:
> 
> > 
> > How does libvirt assign node-names to all the backing images in a
> > qcow2 chain, for example?
> 
> We have the ability to do both command-line and QMP hotplug addition of
> drives where you can set backing.file.node-name (or is it
> backing.node-name?) for any element in a backing chain; libvirt just
> needs to be taught to use it.

backing.node-name and backing.file.node-name usually both exist, and
they are different nodes (one is the qcow2 layer, the other on the
raw-posix layer).

Kevin

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

[Qemu-devel] [PATCH 2/5] block: add helper function to determine if a BDS is in a chain

[Jeff Cody]

This is a small helper function, to determine if 'base' is in the
chain of BlockDriverState 'top'.  It returns true if it is in the chain,
and false otherwise.

If either argument is NULL, it will also return false.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c               | 9 +++++++++
 include/block/block.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/block.c b/block.c
index 81945d3..c4f77c2 100644
--- a/block.c
+++ b/block.c
@@ -3734,6 +3734,15 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
     return NULL;
 }
 
+bool bdrv_is_in_chain(BlockDriverState *top, BlockDriverState *base)
+{
+    while (top && top != base) {
+        top = top->backing_hd;
+    }
+
+    return top != NULL;
+}
+
 BlockDriverState *bdrv_next(BlockDriverState *bs)
 {
     if (!bs) {
diff --git a/include/block/block.h b/include/block/block.h
index 1b119aa..283a6f3 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -381,6 +381,7 @@ BlockDeviceInfoList *bdrv_named_nodes_list(void);
 BlockDriverState *bdrv_lookup_bs(const char *device,
                                  const char *node_name,
                                  Error **errp);
+bool bdrv_is_in_chain(BlockDriverState *top, BlockDriverState *base);
 BlockDriverState *bdrv_next(BlockDriverState *bs);
 void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs),
                   void *opaque);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/5] block: add helper function to determine if a BDS is in a chain

[Benoît Canet]

The Wednesday 14 May 2014 à 23:20:16 (-0400), Jeff Cody wrote :
> This is a small helper function, to determine if 'base' is in the
> chain of BlockDriverState 'top'.  It returns true if it is in the chain,
> and false otherwise.
> 
> If either argument is NULL, it will also return false.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c               | 9 +++++++++
>  include/block/block.h | 1 +
>  2 files changed, 10 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 81945d3..c4f77c2 100644
> --- a/block.c
> +++ b/block.c
> @@ -3734,6 +3734,15 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
>      return NULL;
>  }
>  
> +bool bdrv_is_in_chain(BlockDriverState *top, BlockDriverState *base)
> +{
> +    while (top && top != base) {
> +        top = top->backing_hd;
> +    }
> +
> +    return top != NULL;
> +}
> +
>  BlockDriverState *bdrv_next(BlockDriverState *bs)
>  {
>      if (!bs) {
> diff --git a/include/block/block.h b/include/block/block.h
> index 1b119aa..283a6f3 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -381,6 +381,7 @@ BlockDeviceInfoList *bdrv_named_nodes_list(void);
>  BlockDriverState *bdrv_lookup_bs(const char *device,
>                                   const char *node_name,
>                                   Error **errp);
> +bool bdrv_is_in_chain(BlockDriverState *top, BlockDriverState *base);
>  BlockDriverState *bdrv_next(BlockDriverState *bs);
>  void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs),
>                    void *opaque);
> -- 
> 1.8.3.1
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH 2/5] block: add helper function to determine if a BDS is in a chain

[Eric Blake]

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

On 05/14/2014 09:20 PM, Jeff Cody wrote:
> This is a small helper function, to determine if 'base' is in the
> chain of BlockDriverState 'top'.  It returns true if it is in the chain,
> and false otherwise.
> 
> If either argument is NULL, it will also return false.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c               | 9 +++++++++
>  include/block/block.h | 1 +
>  2 files changed, 10 insertions(+)
> 

>  
> +bool bdrv_is_in_chain(BlockDriverState *top, BlockDriverState *base)

No doc comments inline, and not everyone has the commit message handy.
Which means someone trying to learn what this command does has to read
the function.  Maybe copy the commit message into code comments as well.

Bikeshedding: If I'm reading code, and see bdrv_is_in_chain(a, b), my
first inclination would be to read that as "return true if node a is in
chain b".  But if I were to see bdrv_chain_contains(a, b), I would parse
that as "return true if chain a contains node b".  I think you either
want to swap argument order, or rename the function.

The function itself looks useful, though, once we agree on the naming.

-- 
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 2/5] block: add helper function to determine if a BDS is in a chain

[Kevin Wolf]

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

Am 15.05.2014 um 16:16 hat Eric Blake geschrieben:
> On 05/14/2014 09:20 PM, Jeff Cody wrote:
> > This is a small helper function, to determine if 'base' is in the
> > chain of BlockDriverState 'top'.  It returns true if it is in the chain,
> > and false otherwise.
> > 
> > If either argument is NULL, it will also return false.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block.c               | 9 +++++++++
> >  include/block/block.h | 1 +
> >  2 files changed, 10 insertions(+)
> > 
> 
> >  
> > +bool bdrv_is_in_chain(BlockDriverState *top, BlockDriverState *base)
> 
> No doc comments inline, and not everyone has the commit message handy.
> Which means someone trying to learn what this command does has to read
> the function.  Maybe copy the commit message into code comments as well.
> 
> Bikeshedding: If I'm reading code, and see bdrv_is_in_chain(a, b), my
> first inclination would be to read that as "return true if node a is in
> chain b".  But if I were to see bdrv_chain_contains(a, b), I would parse
> that as "return true if chain a contains node b".  I think you either
> want to swap argument order, or rename the function.

I haven't read the patch yet, so I can't say much about the context, but
just from seeing the function names I think I agree with you.

Kevin

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

Re: [Qemu-devel] [PATCH 2/5] block: add helper function to determine if a BDS is in a chain

[Jeff Cody]

On Thu, May 15, 2014 at 08:16:27AM -0600, Eric Blake wrote:
> On 05/14/2014 09:20 PM, Jeff Cody wrote:
> > This is a small helper function, to determine if 'base' is in the
> > chain of BlockDriverState 'top'.  It returns true if it is in the chain,
> > and false otherwise.
> > 
> > If either argument is NULL, it will also return false.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block.c               | 9 +++++++++
> >  include/block/block.h | 1 +
> >  2 files changed, 10 insertions(+)
> > 
> 
> >  
> > +bool bdrv_is_in_chain(BlockDriverState *top, BlockDriverState *base)
> 
> No doc comments inline, and not everyone has the commit message handy.
> Which means someone trying to learn what this command does has to read
> the function.  Maybe copy the commit message into code comments as well.
> 

OK

> Bikeshedding: If I'm reading code, and see bdrv_is_in_chain(a, b), my
> first inclination would be to read that as "return true if node a is in
> chain b".  But if I were to see bdrv_chain_contains(a, b), I would parse
> that as "return true if chain a contains node b".  I think you either
> want to swap argument order, or rename the function.

I like the bdrv_chain_contains() as the function name, it is clearer.
I'll use that.

> 
> The function itself looks useful, though, once we agree on the naming.
> 

[Qemu-devel] [PATCH 3/5] block: make 'top' argument to block-commit optional

[Jeff Cody]

Now that active layer block-commit is supported, the 'top' argument
no longer needs to be mandatory.

Change it optional, with the default being the active layer in the
device chain.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 blockdev.c             |  3 ++-
 qapi-schema.json       |  7 ++++---
 qmp-commands.hx        |  5 +++--
 tests/qemu-iotests/040 | 28 ++++++++++++++++++----------
 4 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 500707e..02c6525 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1868,7 +1868,8 @@ void qmp_block_stream(const char *device, bool has_base,
 }
 
 void qmp_block_commit(const char *device,
-                      bool has_base, const char *base, const char *top,
+                      bool has_base, const char *base,
+                      bool has_top, const char *top,
                       bool has_speed, int64_t speed,
                       Error **errp)
 {
diff --git a/qapi-schema.json b/qapi-schema.json
index 36cb964..06a9b5d 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2099,8 +2099,9 @@
 # @base:   #optional The file name of the backing image to write data into.
 #                    If not specified, this is the deepest backing image
 #
-# @top:              The file name of the backing image within the image chain,
-#                    which contains the topmost data to be committed down.
+# @top:    #optional 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.
 #
 #                    If top == base, that is an error.
 #                    If top == active, the job will not be completed by itself,
@@ -2128,7 +2129,7 @@
 #
 ##
 { 'command': 'block-commit',
-  'data': { 'device': 'str', '*base': 'str', 'top': 'str',
+  'data': { 'device': 'str', '*base': 'str', '*top': 'str',
             '*speed': 'int' } }
 
 ##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index cae890e..1aa3c65 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -985,7 +985,7 @@ EQMP
 
     {
         .name       = "block-commit",
-        .args_type  = "device:B,base:s?,top:s,speed:o?",
+        .args_type  = "device:B,base:s?,top:s?,speed:o?",
         .mhandler.cmd_new = qmp_marshal_input_block_commit,
     },
 
@@ -1003,7 +1003,8 @@ Arguments:
           If not specified, this is the deepest backing image
           (json-string, optional)
 - "top":  The file name of the backing image within the image chain,
-          which contains the topmost data to be committed down.
+          which contains the topmost data to be committed down. If
+          not specified, this is the active layer. (json-string, optional)
 
           If top == base, that is an error.
           If top == active, the job will not be completed by itself,
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 734b6a6..803b0c7 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -35,11 +35,7 @@ test_img = os.path.join(iotests.test_dir, 'test.img')
 class ImageCommitTestCase(iotests.QMPTestCase):
     '''Abstract base class for image commit test cases'''
 
-    def run_commit_test(self, top, base):
-        self.assert_no_active_block_jobs()
-        result = self.vm.qmp('block-commit', device='drive0', top=top, base=base)
-        self.assert_qmp(result, 'return', {})
-
+    def wait_for_complete(self):
         completed = False
         while not completed:
             for event in self.vm.get_qmp_events(wait=True):
@@ -58,6 +54,18 @@ class ImageCommitTestCase(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
+    def run_commit_test(self, top, base):
+        self.assert_no_active_block_jobs()
+        result = self.vm.qmp('block-commit', device='drive0', top=top, base=base)
+        self.assert_qmp(result, 'return', {})
+        self.wait_for_complete()
+
+    def run_default_commit_test(self):
+        self.assert_no_active_block_jobs()
+        result = self.vm.qmp('block-commit', device='drive0')
+        self.assert_qmp(result, 'return', {})
+        self.wait_for_complete()
+
 class TestSingleDrive(ImageCommitTestCase):
     image_len = 1 * 1024 * 1024
     test_len = 1 * 1024 * 256
@@ -109,17 +117,17 @@ class TestSingleDrive(ImageCommitTestCase):
         self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
         self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
 
+    def test_top_is_default_active(self):
+        self.run_default_commit_test()
+        self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
+        self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
+
     def test_top_and_base_reversed(self):
         self.assert_no_active_block_jobs()
         result = self.vm.qmp('block-commit', device='drive0', top='%s' % backing_img, base='%s' % mid_img)
         self.assert_qmp(result, 'error/class', 'GenericError')
         self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % mid_img)
 
-    def test_top_omitted(self):
-        self.assert_no_active_block_jobs()
-        result = self.vm.qmp('block-commit', device='drive0')
-        self.assert_qmp(result, 'error/class', 'GenericError')
-        self.assert_qmp(result, 'error/desc', "Parameter 'top' is missing")
 
 class TestRelativePaths(ImageCommitTestCase):
     image_len = 1 * 1024 * 1024
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 3/5] block: make 'top' argument to block-commit optional

[Benoît Canet]

The Wednesday 14 May 2014 à 23:20:17 (-0400), Jeff Cody wrote :
> Now that active layer block-commit is supported, the 'top' argument
> no longer needs to be mandatory.
> 
> Change it optional, with the default being the active layer in the

Do you mean "Change it to optional" or "Make it optional" ? 

> device chain.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  blockdev.c             |  3 ++-
>  qapi-schema.json       |  7 ++++---
>  qmp-commands.hx        |  5 +++--
>  tests/qemu-iotests/040 | 28 ++++++++++++++++++----------
>  4 files changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 500707e..02c6525 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1868,7 +1868,8 @@ void qmp_block_stream(const char *device, bool has_base,
>  }
>  
>  void qmp_block_commit(const char *device,
> -                      bool has_base, const char *base, const char *top,
> +                      bool has_base, const char *base,
> +                      bool has_top, const char *top,
>                        bool has_speed, int64_t speed,
>                        Error **errp)
>  {
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 36cb964..06a9b5d 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2099,8 +2099,9 @@
>  # @base:   #optional The file name of the backing image to write data into.
>  #                    If not specified, this is the deepest backing image
>  #
> -# @top:              The file name of the backing image within the image chain,
> -#                    which contains the topmost data to be committed down.
> +# @top:    #optional 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.
>  #
>  #                    If top == base, that is an error.
>  #                    If top == active, the job will not be completed by itself,
> @@ -2128,7 +2129,7 @@
>  #
>  ##
>  { 'command': 'block-commit',
> -  'data': { 'device': 'str', '*base': 'str', 'top': 'str',
> +  'data': { 'device': 'str', '*base': 'str', '*top': 'str',
>              '*speed': 'int' } }
>  
>  ##
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index cae890e..1aa3c65 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -985,7 +985,7 @@ EQMP
>  
>      {
>          .name       = "block-commit",
> -        .args_type  = "device:B,base:s?,top:s,speed:o?",
> +        .args_type  = "device:B,base:s?,top:s?,speed:o?",
>          .mhandler.cmd_new = qmp_marshal_input_block_commit,
>      },
>  
> @@ -1003,7 +1003,8 @@ Arguments:
>            If not specified, this is the deepest backing image
>            (json-string, optional)
>  - "top":  The file name of the backing image within the image chain,
> -          which contains the topmost data to be committed down.
> +          which contains the topmost data to be committed down. If
> +          not specified, this is the active layer. (json-string, optional)
>  
>            If top == base, that is an error.
>            If top == active, the job will not be completed by itself,
> diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
> index 734b6a6..803b0c7 100755
> --- a/tests/qemu-iotests/040
> +++ b/tests/qemu-iotests/040
> @@ -35,11 +35,7 @@ test_img = os.path.join(iotests.test_dir, 'test.img')
>  class ImageCommitTestCase(iotests.QMPTestCase):
>      '''Abstract base class for image commit test cases'''
>  
> -    def run_commit_test(self, top, base):
> -        self.assert_no_active_block_jobs()
> -        result = self.vm.qmp('block-commit', device='drive0', top=top, base=base)
> -        self.assert_qmp(result, 'return', {})
> -
> +    def wait_for_complete(self):
>          completed = False
>          while not completed:
>              for event in self.vm.get_qmp_events(wait=True):
> @@ -58,6 +54,18 @@ class ImageCommitTestCase(iotests.QMPTestCase):
>          self.assert_no_active_block_jobs()
>          self.vm.shutdown()
>  
> +    def run_commit_test(self, top, base):
> +        self.assert_no_active_block_jobs()
> +        result = self.vm.qmp('block-commit', device='drive0', top=top, base=base)
> +        self.assert_qmp(result, 'return', {})
> +        self.wait_for_complete()
> +
> +    def run_default_commit_test(self):
> +        self.assert_no_active_block_jobs()
> +        result = self.vm.qmp('block-commit', device='drive0')
> +        self.assert_qmp(result, 'return', {})
> +        self.wait_for_complete()
> +
>  class TestSingleDrive(ImageCommitTestCase):
>      image_len = 1 * 1024 * 1024
>      test_len = 1 * 1024 * 256
> @@ -109,17 +117,17 @@ class TestSingleDrive(ImageCommitTestCase):
>          self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
>          self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
>  
> +    def test_top_is_default_active(self):
> +        self.run_default_commit_test()
> +        self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
> +        self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
> +
>      def test_top_and_base_reversed(self):
>          self.assert_no_active_block_jobs()
>          result = self.vm.qmp('block-commit', device='drive0', top='%s' % backing_img, base='%s' % mid_img)
>          self.assert_qmp(result, 'error/class', 'GenericError')
>          self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % mid_img)
>  
> -    def test_top_omitted(self):
> -        self.assert_no_active_block_jobs()
> -        result = self.vm.qmp('block-commit', device='drive0')
> -        self.assert_qmp(result, 'error/class', 'GenericError')
> -        self.assert_qmp(result, 'error/desc', "Parameter 'top' is missing")
>  
>  class TestRelativePaths(ImageCommitTestCase):
>      image_len = 1 * 1024 * 1024
> -- 
> 1.8.3.1
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH 3/5] block: make 'top' argument to block-commit optional

[Jeff Cody]

On Thu, May 15, 2014 at 01:47:55PM +0200, Benoît Canet wrote:
> The Wednesday 14 May 2014 à 23:20:17 (-0400), Jeff Cody wrote :
> > Now that active layer block-commit is supported, the 'top' argument
> > no longer needs to be mandatory.
> > 
> > Change it optional, with the default being the active layer in the
> 
> Do you mean "Change it to optional" or "Make it optional" ? 
>

Thanks - forgot the "to".

> > device chain.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  blockdev.c             |  3 ++-
> >  qapi-schema.json       |  7 ++++---
> >  qmp-commands.hx        |  5 +++--
> >  tests/qemu-iotests/040 | 28 ++++++++++++++++++----------
> >  4 files changed, 27 insertions(+), 16 deletions(-)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index 500707e..02c6525 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -1868,7 +1868,8 @@ void qmp_block_stream(const char *device, bool has_base,
> >  }
> >  
> >  void qmp_block_commit(const char *device,
> > -                      bool has_base, const char *base, const char *top,
> > +                      bool has_base, const char *base,
> > +                      bool has_top, const char *top,
> >                        bool has_speed, int64_t speed,
> >                        Error **errp)
> >  {
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 36cb964..06a9b5d 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -2099,8 +2099,9 @@
> >  # @base:   #optional The file name of the backing image to write data into.
> >  #                    If not specified, this is the deepest backing image
> >  #
> > -# @top:              The file name of the backing image within the image chain,
> > -#                    which contains the topmost data to be committed down.
> > +# @top:    #optional 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.
> >  #
> >  #                    If top == base, that is an error.
> >  #                    If top == active, the job will not be completed by itself,
> > @@ -2128,7 +2129,7 @@
> >  #
> >  ##
> >  { 'command': 'block-commit',
> > -  'data': { 'device': 'str', '*base': 'str', 'top': 'str',
> > +  'data': { 'device': 'str', '*base': 'str', '*top': 'str',
> >              '*speed': 'int' } }
> >  
> >  ##
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index cae890e..1aa3c65 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -985,7 +985,7 @@ EQMP
> >  
> >      {
> >          .name       = "block-commit",
> > -        .args_type  = "device:B,base:s?,top:s,speed:o?",
> > +        .args_type  = "device:B,base:s?,top:s?,speed:o?",
> >          .mhandler.cmd_new = qmp_marshal_input_block_commit,
> >      },
> >  
> > @@ -1003,7 +1003,8 @@ Arguments:
> >            If not specified, this is the deepest backing image
> >            (json-string, optional)
> >  - "top":  The file name of the backing image within the image chain,
> > -          which contains the topmost data to be committed down.
> > +          which contains the topmost data to be committed down. If
> > +          not specified, this is the active layer. (json-string, optional)
> >  
> >            If top == base, that is an error.
> >            If top == active, the job will not be completed by itself,
> > diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
> > index 734b6a6..803b0c7 100755
> > --- a/tests/qemu-iotests/040
> > +++ b/tests/qemu-iotests/040
> > @@ -35,11 +35,7 @@ test_img = os.path.join(iotests.test_dir, 'test.img')
> >  class ImageCommitTestCase(iotests.QMPTestCase):
> >      '''Abstract base class for image commit test cases'''
> >  
> > -    def run_commit_test(self, top, base):
> > -        self.assert_no_active_block_jobs()
> > -        result = self.vm.qmp('block-commit', device='drive0', top=top, base=base)
> > -        self.assert_qmp(result, 'return', {})
> > -
> > +    def wait_for_complete(self):
> >          completed = False
> >          while not completed:
> >              for event in self.vm.get_qmp_events(wait=True):
> > @@ -58,6 +54,18 @@ class ImageCommitTestCase(iotests.QMPTestCase):
> >          self.assert_no_active_block_jobs()
> >          self.vm.shutdown()
> >  
> > +    def run_commit_test(self, top, base):
> > +        self.assert_no_active_block_jobs()
> > +        result = self.vm.qmp('block-commit', device='drive0', top=top, base=base)
> > +        self.assert_qmp(result, 'return', {})
> > +        self.wait_for_complete()
> > +
> > +    def run_default_commit_test(self):
> > +        self.assert_no_active_block_jobs()
> > +        result = self.vm.qmp('block-commit', device='drive0')
> > +        self.assert_qmp(result, 'return', {})
> > +        self.wait_for_complete()
> > +
> >  class TestSingleDrive(ImageCommitTestCase):
> >      image_len = 1 * 1024 * 1024
> >      test_len = 1 * 1024 * 256
> > @@ -109,17 +117,17 @@ class TestSingleDrive(ImageCommitTestCase):
> >          self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
> >          self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
> >  
> > +    def test_top_is_default_active(self):
> > +        self.run_default_commit_test()
> > +        self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
> > +        self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
> > +
> >      def test_top_and_base_reversed(self):
> >          self.assert_no_active_block_jobs()
> >          result = self.vm.qmp('block-commit', device='drive0', top='%s' % backing_img, base='%s' % mid_img)
> >          self.assert_qmp(result, 'error/class', 'GenericError')
> >          self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % mid_img)
> >  
> > -    def test_top_omitted(self):
> > -        self.assert_no_active_block_jobs()
> > -        result = self.vm.qmp('block-commit', device='drive0')
> > -        self.assert_qmp(result, 'error/class', 'GenericError')
> > -        self.assert_qmp(result, 'error/desc', "Parameter 'top' is missing")
> >  
> >  class TestRelativePaths(ImageCommitTestCase):
> >      image_len = 1 * 1024 * 1024
> > -- 
> > 1.8.3.1
> > 
> Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH 3/5] block: make 'top' argument to block-commit optional

[Eric Blake]

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

On 05/14/2014 09:20 PM, Jeff Cody wrote:
> Now that active layer block-commit is supported, the 'top' argument
> no longer needs to be mandatory.
> 
> Change it optional, with the default being the active layer in the
> device chain.

Good, this is an API-compatible change (doesn't break old clients that
always provide 'top').  I'm definitely in favor of it.

> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  blockdev.c             |  3 ++-
>  qapi-schema.json       |  7 ++++---
>  qmp-commands.hx        |  5 +++--
>  tests/qemu-iotests/040 | 28 ++++++++++++++++++----------
>  4 files changed, 27 insertions(+), 16 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -2099,8 +2099,9 @@
>  # @base:   #optional The file name of the backing image to write data into.
>  #                    If not specified, this is the deepest backing image
>  #
> -# @top:              The file name of the backing image within the image chain,
> -#                    which contains the topmost data to be committed down.
> +# @top:    #optional 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.

We have not done the conversion from mandatory to optional very often; I
wonder if it is worth an annotation of when the conversion happened (so
someone reading just this version of the schema but planning on
targeting older qemu knows to always supply the argument).  Something like:

@top: since 1.3, #optional since 2.1; The file name...

But that feels rather long.  It's not a show-stopper to me, particularly
since we still don't have any automated conversion from these comments
into any other documentation formats.  That is, if we decide it is worth
adding hints about when something changed from mandatory to optional, it
would be its own patch and cover the work we've done on other recent
changes like block_passwd.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 --]

[Qemu-devel] [PATCH 4/5] block: Accept node-name arguments for block-commit

[Jeff Cody]

This modifies the block operation block-commit so that it will
accept node-name arguments for either 'top' or 'base' BDS.

The filename and node-name are mutually exclusive to each other;
i.e.:
    "top" and "top-node-name" are mutually exclusive (enforced)
    "base" and "base-node-name" are mutually exclusive (enforced)

The preferred and recommended method now of using block-commit
is to specify the BDS to operate on via their node-name arguments.

This also adds an explicit check that base_bs is in the chain of
top_bs, because if they are referenced by their unique node-name
arguments, the discovery process does not intrinsically verify
they are in the same chain.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 blockdev.c       | 35 +++++++++++++++++++++++++++++++++--
 qapi-schema.json | 35 ++++++++++++++++++++++++++---------
 qmp-commands.hx  | 29 ++++++++++++++++++++++-------
 3 files changed, 81 insertions(+), 18 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 02c6525..d8cb396 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1869,7 +1869,9 @@ void qmp_block_stream(const char *device, bool has_base,
 
 void qmp_block_commit(const char *device,
                       bool has_base, const char *base,
+                      bool has_base_node_name, const char *base_node_name,
                       bool has_top, const char *top,
+                      bool has_top_node_name, const char *top_node_name,
                       bool has_speed, int64_t speed,
                       Error **errp)
 {
@@ -1888,6 +1890,15 @@ void qmp_block_commit(const char *device,
     /* drain all i/o before commits */
     bdrv_drain_all();
 
+    if (has_base && has_base_node_name) {
+        error_setg(errp, "'base' and 'base-node-name' are mutually exclusive");
+        return;
+    }
+    if (has_top && has_top_node_name) {
+        error_setg(errp, "'top' and 'top-node-name' are mutually exclusive");
+        return;
+    }
+
     bs = bdrv_find(device);
     if (!bs) {
         error_set(errp, QERR_DEVICE_NOT_FOUND, device);
@@ -1897,7 +1908,14 @@ void qmp_block_commit(const char *device,
     /* default top_bs is the active layer */
     top_bs = bs;
 
-    if (top) {
+    /* Find the 'top' image file for the commit */
+    if (has_top_node_name) {
+        top_bs = bdrv_lookup_bs(NULL, top_node_name, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    } else if (has_top && top) {
         if (strcmp(bs->filename, top) != 0) {
             top_bs = bdrv_find_backing_image(bs, top);
         }
@@ -1908,7 +1926,14 @@ void qmp_block_commit(const char *device,
         return;
     }
 
-    if (has_base && base) {
+    /* Find the 'base' image file for the commit */
+    if (has_base_node_name) {
+        base_bs = bdrv_lookup_bs(NULL, base_node_name, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    } else if (has_base && base) {
         base_bs = bdrv_find_backing_image(top_bs, base);
     } else {
         base_bs = bdrv_find_base(top_bs);
@@ -1919,6 +1944,12 @@ void qmp_block_commit(const char *device,
         return;
     }
 
+    /* Verify that 'base' is in the same chain as 'top' */
+    if (!bdrv_is_in_chain(top_bs, base_bs)) {
+        error_setg(errp, "'base' and 'top' are not in the same chain");
+        return;
+    }
+
     if (top_bs == bs) {
         commit_active_start(bs, base_bs, speed, on_error, block_job_cb,
                             bs, &local_err);
diff --git a/qapi-schema.json b/qapi-schema.json
index 06a9b5d..eddb2b8 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2096,12 +2096,27 @@
 #
 # @device:  the name of the device
 #
-# @base:   #optional The file name of the backing image to write data into.
-#                    If not specified, this is the deepest backing image
+# For 'base', either @base or @base-node-name may be set but not both. If
+# neither is specified, this is the deepest backing image
 #
-# @top:    #optional 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:          #optional The file name of the backing image to write data
+#                           into.
+#
+# @base-node-name #optional The name of the block driver state node of the
+#                           backing image to write data into.
+#                           (Since 2.1)
+#
+# For 'top', either @top or @top-node-name must be set but not both.
+#
+# @top:           #optional 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.
+#
+# @top-node-name: #optional The block driver state node name of the backing
+#                           image within the image chain, which contains the
+#                           topmost data to be committed down.
+#                           (Since 2.1)
 #
 #                    If top == base, that is an error.
 #                    If top == active, the job will not be completed by itself,
@@ -2120,17 +2135,19 @@
 #
 # Returns: Nothing on success
 #          If commit or stream is already active on this device, DeviceInUse
-#          If @device does not exist, DeviceNotFound
+#          If @device does not exist or cannot be determined, DeviceNotFound
 #          If image commit is not supported by this device, NotSupported
-#          If @base or @top is invalid, a generic error is returned
+#          If @base, @top, @base-node-name, @top-node-name invalid, GenericError
 #          If @speed is invalid, InvalidParameter
+#          If both @base and @base-node-name are specified, GenericError
+#          If both @top and @top-node-name are specified, GenericError
 #
 # Since: 1.3
 #
 ##
 { 'command': 'block-commit',
-  'data': { 'device': 'str', '*base': 'str', '*top': 'str',
-            '*speed': 'int' } }
+  'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str',
+            '*top': 'str', '*top-node-name': 'str', '*speed': 'int' } }
 
 ##
 # @drive-backup
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 1aa3c65..2b9b1dc 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -985,7 +985,7 @@ EQMP
 
     {
         .name       = "block-commit",
-        .args_type  = "device:B,base:s?,top:s?,speed:o?",
+        .args_type  = "device:B,base:s?,base-node-name:s?,top:s?,top-node-name:s?,speed:o?",
         .mhandler.cmd_new = qmp_marshal_input_block_commit,
     },
 
@@ -999,12 +999,27 @@ data between 'top' and 'base' into 'base'.
 Arguments:
 
 - "device": The device's ID, must be unique (json-string)
-- "base": The file name of the backing image to write data into.
-          If not specified, this is the deepest backing image
-          (json-string, optional)
-- "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. (json-string, optional)
+
+For 'base', either base or base-node-name may be set but not both. If
+neither is specified, this is the deepest backing image
+
+- "base":           The file name of the backing image to write data into.
+                    (json-string, optional)
+- "base-node-name": The name of the block driver state node of the
+                    backing image to write data into.
+                    (json-string, optional) (Since 2.1)
+
+For 'top', either top or top-node-name must be set but not both.
+
+- "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.
+                    (json-string, optional)
+
+- "top-node-name": The block driver state node name of the backing
+                   image within the image chain, which contains the
+                   topmost data to be committed down.
+                   (json-string, optional) (Since 2.1)
 
           If top == base, that is an error.
           If top == active, the job will not be completed by itself,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 4/5] block: Accept node-name arguments for block-commit

[Benoît Canet]

The Wednesday 14 May 2014 à 23:20:18 (-0400), Jeff Cody wrote :
> This modifies the block operation block-commit so that it will
> accept node-name arguments for either 'top' or 'base' BDS.
> 
> The filename and node-name are mutually exclusive to each other;
> i.e.:
>     "top" and "top-node-name" are mutually exclusive (enforced)
>     "base" and "base-node-name" are mutually exclusive (enforced)
> 
> The preferred and recommended method now of using block-commit
> is to specify the BDS to operate on via their node-name arguments.
> 
> This also adds an explicit check that base_bs is in the chain of
> top_bs, because if they are referenced by their unique node-name
> arguments, the discovery process does not intrinsically verify
> they are in the same chain.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  blockdev.c       | 35 +++++++++++++++++++++++++++++++++--
>  qapi-schema.json | 35 ++++++++++++++++++++++++++---------
>  qmp-commands.hx  | 29 ++++++++++++++++++++++-------
>  3 files changed, 81 insertions(+), 18 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 02c6525..d8cb396 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1869,7 +1869,9 @@ void qmp_block_stream(const char *device, bool has_base,
>  
>  void qmp_block_commit(const char *device,
>                        bool has_base, const char *base,
> +                      bool has_base_node_name, const char *base_node_name,
>                        bool has_top, const char *top,
> +                      bool has_top_node_name, const char *top_node_name,
>                        bool has_speed, int64_t speed,
>                        Error **errp)
>  {
> @@ -1888,6 +1890,15 @@ void qmp_block_commit(const char *device,
>      /* drain all i/o before commits */
>      bdrv_drain_all();
>  
> +    if (has_base && has_base_node_name) {
> +        error_setg(errp, "'base' and 'base-node-name' are mutually exclusive");
> +        return;
> +    }
> +    if (has_top && has_top_node_name) {
> +        error_setg(errp, "'top' and 'top-node-name' are mutually exclusive");
> +        return;
> +    }
> +
>      bs = bdrv_find(device);
>      if (!bs) {
>          error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> @@ -1897,7 +1908,14 @@ void qmp_block_commit(const char *device,
>      /* default top_bs is the active layer */
>      top_bs = bs;
>  
> -    if (top) {
> +    /* Find the 'top' image file for the commit */
> +    if (has_top_node_name) {
> +        top_bs = bdrv_lookup_bs(NULL, top_node_name, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    } else if (has_top && top) {
>          if (strcmp(bs->filename, top) != 0) {
>              top_bs = bdrv_find_backing_image(bs, top);
>          }
> @@ -1908,7 +1926,14 @@ void qmp_block_commit(const char *device,
>          return;
>      }
>  
> -    if (has_base && base) {
> +    /* Find the 'base' image file for the commit */
> +    if (has_base_node_name) {
> +        base_bs = bdrv_lookup_bs(NULL, base_node_name, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    } else if (has_base && base) {
>          base_bs = bdrv_find_backing_image(top_bs, base);
>      } else {
>          base_bs = bdrv_find_base(top_bs);
> @@ -1919,6 +1944,12 @@ void qmp_block_commit(const char *device,
>          return;
>      }
>  
> +    /* Verify that 'base' is in the same chain as 'top' */
> +    if (!bdrv_is_in_chain(top_bs, base_bs)) {
> +        error_setg(errp, "'base' and 'top' are not in the same chain");
> +        return;
> +    }
> +
>      if (top_bs == bs) {
>          commit_active_start(bs, base_bs, speed, on_error, block_job_cb,
>                              bs, &local_err);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 06a9b5d..eddb2b8 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2096,12 +2096,27 @@
>  #
>  # @device:  the name of the device
>  #
> -# @base:   #optional The file name of the backing image to write data into.
> -#                    If not specified, this is the deepest backing image
> +# For 'base', either @base or @base-node-name may be set but not both. If
> +# neither is specified, this is the deepest backing image
>  #
> -# @top:    #optional 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:          #optional The file name of the backing image to write data
> +#                           into.
> +#
> +# @base-node-name #optional The name of the block driver state node of the
> +#                           backing image to write data into.
> +#                           (Since 2.1)
> +#
> +# For 'top', either @top or @top-node-name must be set but not both.
> +#
> +# @top:           #optional 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.
> +#
> +# @top-node-name: #optional The block driver state node name of the backing
> +#                           image within the image chain, which contains the
> +#                           topmost data to be committed down.
> +#                           (Since 2.1)
>  #
>  #                    If top == base, that is an error.
>  #                    If top == active, the job will not be completed by itself,
> @@ -2120,17 +2135,19 @@
>  #
>  # Returns: Nothing on success
>  #          If commit or stream is already active on this device, DeviceInUse
> -#          If @device does not exist, DeviceNotFound
> +#          If @device does not exist or cannot be determined, DeviceNotFound
>  #          If image commit is not supported by this device, NotSupported
> -#          If @base or @top is invalid, a generic error is returned
> +#          If @base, @top, @base-node-name, @top-node-name invalid, GenericError
>  #          If @speed is invalid, InvalidParameter
> +#          If both @base and @base-node-name are specified, GenericError
> +#          If both @top and @top-node-name are specified, GenericError
>  #
>  # Since: 1.3
>  #
>  ##
>  { 'command': 'block-commit',
> -  'data': { 'device': 'str', '*base': 'str', '*top': 'str',
> -            '*speed': 'int' } }
> +  'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str',
> +            '*top': 'str', '*top-node-name': 'str', '*speed': 'int' } }
>  
>  ##
>  # @drive-backup
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 1aa3c65..2b9b1dc 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -985,7 +985,7 @@ EQMP
>  
>      {
>          .name       = "block-commit",
> -        .args_type  = "device:B,base:s?,top:s?,speed:o?",
> +        .args_type  = "device:B,base:s?,base-node-name:s?,top:s?,top-node-name:s?,speed:o?",
>          .mhandler.cmd_new = qmp_marshal_input_block_commit,
>      },
>  
> @@ -999,12 +999,27 @@ data between 'top' and 'base' into 'base'.
>  Arguments:
>  
>  - "device": The device's ID, must be unique (json-string)
> -- "base": The file name of the backing image to write data into.
> -          If not specified, this is the deepest backing image
> -          (json-string, optional)
> -- "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. (json-string, optional)
> +
> +For 'base', either base or base-node-name may be set but not both. If
> +neither is specified, this is the deepest backing image
> +
> +- "base":           The file name of the backing image to write data into.
> +                    (json-string, optional)
> +- "base-node-name": The name of the block driver state node of the
> +                    backing image to write data into.
> +                    (json-string, optional) (Since 2.1)
> +
> +For 'top', either top or top-node-name must be set but not both.
> +
> +- "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.
> +                    (json-string, optional)
> +
> +- "top-node-name": The block driver state node name of the backing
> +                   image within the image chain, which contains the
> +                   topmost data to be committed down.
> +                   (json-string, optional) (Since 2.1)

                          ^ (cosmetic) this block is not aligned with the upper ones.
>  
>            If top == base, that is an error.
>            If top == active, the job will not be completed by itself,
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [PATCH 4/5] block: Accept node-name arguments for block-commit

[Eric Blake]

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

On 05/14/2014 09:20 PM, Jeff Cody wrote:
> This modifies the block operation block-commit so that it will
> accept node-name arguments for either 'top' or 'base' BDS.
> 
> The filename and node-name are mutually exclusive to each other;
> i.e.:
>     "top" and "top-node-name" are mutually exclusive (enforced)
>     "base" and "base-node-name" are mutually exclusive (enforced)

Hmm - we have a bug in existing commands for NOT forcing mutual
exclusion even though the schema says they are exclusive.  For example,
qmp_block_passwd() blindly calls:

    bs = bdrv_lookup_bs(has_device ? device : NULL,
                        has_node_name ? node_name : NULL,
                        &local_err);

and _that_ function blindly favors device name over node name, rather
than erroring out if both are supplied.  I think we should fix that
first - I'd rather that bdrv_lookup_bs either errors out if both names
are supplied (rather than making each caller repeat the work), or that
it checks that if both names are supplied then it resolves to the same bs.

Hmm - if I have device "foo" with chain "base <- top", would
bdrv_lookup_bs("foo", "base") be okay ("base" is a node-name that
resides within "device"'s chain) or an error ("base" resolves to a
different bs than "device")?  Again, all the more reason to have a
common function decide the semantics we want, then all other clients
automatically pick up on those semantics.

> 
> The preferred and recommended method now of using block-commit
> is to specify the BDS to operate on via their node-name arguments.
> 
> This also adds an explicit check that base_bs is in the chain of
> top_bs, because if they are referenced by their unique node-name
> arguments, the discovery process does not intrinsically verify
> they are in the same chain.

I like this addition.

> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  blockdev.c       | 35 +++++++++++++++++++++++++++++++++--
>  qapi-schema.json | 35 ++++++++++++++++++++++++++---------
>  qmp-commands.hx  | 29 ++++++++++++++++++++++-------
>  3 files changed, 81 insertions(+), 18 deletions(-)
> 



>  
> -    if (top) {
> +    /* Find the 'top' image file for the commit */
> +    if (has_top_node_name) {
> +        top_bs = bdrv_lookup_bs(NULL, top_node_name, &local_err);

Hmm.  Shouldn't you ALSO verify that 'top_bs' belongs to the chain owned
by 'device'?  Later on, you verify that 'base_bs' belongs to 'top_bs',
but if I pass node names, those names could have found a top unrelated
to 'device'; and base related to that top.

Maybe that gives more weight to my idea of possibly allowing
bdrv_lookup_bs(device, top_node_name), where the lookup succeeds even
when device and top_node_name resolve to different bs, but where
top_node_name is a node in the chain of device.

> 
> -    if (has_base && base) {
> +    /* Find the 'base' image file for the commit */
> +    if (has_base_node_name) {
> +        base_bs = bdrv_lookup_bs(NULL, base_node_name, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    } else if (has_base && base) {

Here, it doesn't matter whether you pass device or NULL for the device
name...

>  
> +    /* Verify that 'base' is in the same chain as 'top' */
> +    if (!bdrv_is_in_chain(top_bs, base_bs)) {
> +        error_setg(errp, "'base' and 'top' are not in the same chain");
> +        return;

...because this check is still mandatory to prove that a user with chain:
base <- sn1 <- sn2 <- active

is not calling 'device':'active', 'top-node-name':'sn1',
'base-node-name':'sn2' (all that bdrv_lookup_bs can do is verify that
base-node-name belongs to device, not that it _also_ belongs to top_bs).


> -# @base:   #optional The file name of the backing image to write data into.
> -#                    If not specified, this is the deepest backing image
> +# For 'base', either @base or @base-node-name may be set but not both. If
> +# neither is specified, this is the deepest backing image

I like how you factored th is out up front...

>  #
> -# @top:    #optional 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:          #optional The file name of the backing image to write data
> +#                           into.
> +#
> +# @base-node-name #optional The name of the block driver state node of the
> +#                           backing image to write data into.
> +#                           (Since 2.1)
> +#
> +# For 'top', either @top or @top-node-name must be set but not both.

...but here, you were not consistent.  I'd add "If neither is specified,
this is the active image" here...

> +#
> +# @top:           #optional 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.

...and drop the second sentence from here.

> +#
> +# @top-node-name: #optional The block driver state node name of the backing
> +#                           image within the image chain, which contains the
> +#                           topmost data to be committed down.
> +#                           (Since 2.1)
>  #
>  #                    If top == base, that is an error.
>  #                    If top == active, the job will not be completed by itself,
> @@ -2120,17 +2135,19 @@
>  #
>  # Returns: Nothing on success
>  #          If commit or stream is already active on this device, DeviceInUse
> -#          If @device does not exist, DeviceNotFound
> +#          If @device does not exist or cannot be determined, DeviceNotFound
>  #          If image commit is not supported by this device, NotSupported
> -#          If @base or @top is invalid, a generic error is returned
> +#          If @base, @top, @base-node-name, @top-node-name invalid, GenericError
>  #          If @speed is invalid, InvalidParameter
> +#          If both @base and @base-node-name are specified, GenericError
> +#          If both @top and @top-node-name are specified, GenericError

These last two are arguably sub-conditions of the earlier statement
about @top and @top-node-name being invalid (since being invalid can
include when both strings are used at once).  It wouldn't hurt my
feelings to reduce the docs by two lines.

>  # Since: 1.3
>  #
>  ##
>  { 'command': 'block-commit',
> -  'data': { 'device': 'str', '*base': 'str', '*top': 'str',
> -            '*speed': 'int' } }
> +  'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str',
> +            '*top': 'str', '*top-node-name': 'str', '*speed': 'int' } }

I'm okay with keeping 'device' mandatory, even though technically the
use of a node-name could imply which device is intended.  That is, as
long as the code correctly errors out when device and top-node-name
don't resolve to the same chain :)


> +
> +For 'top', either top or top-node-name must be set but not both.
> +
> +- "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.
> +                    (json-string, optional)

Same blurb about hoisting the 'not specified => active' sentence to the
common text, rather than leaving it in the 'top' text.

-- 
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 4/5] block: Accept node-name arguments for block-commit

[Jeff Cody]

On Thu, May 15, 2014 at 09:42:07AM -0600, Eric Blake wrote:
> On 05/14/2014 09:20 PM, Jeff Cody wrote:
> > This modifies the block operation block-commit so that it will
> > accept node-name arguments for either 'top' or 'base' BDS.
> > 
> > The filename and node-name are mutually exclusive to each other;
> > i.e.:
> >     "top" and "top-node-name" are mutually exclusive (enforced)
> >     "base" and "base-node-name" are mutually exclusive (enforced)
> 
> Hmm - we have a bug in existing commands for NOT forcing mutual
> exclusion even though the schema says they are exclusive.  

Those are a slightly different scenario, however.  In this patch, we
are dealing with a filename string, and a node-name string.  There is
more of a 1:1 relationship between those two (both identify a
particular image).  The other existing commands, however, deal with a
device name string (identifying a drive chain) or a node-name string
(identifying a particular image).

That said, I think the other commands should be modified to enforce
the documentation, but it doesn't really relate to this patch.

> For example,
> qmp_block_passwd() blindly calls:
> 
>     bs = bdrv_lookup_bs(has_device ? device : NULL,
>                         has_node_name ? node_name : NULL,
>                         &local_err);
> 
> and _that_ function blindly favors device name over node name, rather
> than erroring out if both are supplied.  

I find this an odd function to begin with, because node_name uniquely
identifies any given BDS from any chain.  Device name will uniquely
identify a whole chain, by returning its top-most BDS.

The function seems confused.


> I think we should fix that
> first - I'd rather that bdrv_lookup_bs either errors out if both names
> are supplied (rather than making each caller repeat the work), or that
> it checks that if both names are supplied then it resolves to the same bs.
> 

I disagree on this part, at least partially.  It think it needs to be
changed, but I don't think it needs to be part of this series.  I
think that can be a separate series, to address that across the other
QAPI commands that accept node-name and device name


> Hmm - if I have device "foo" with chain "base <- top", would
> bdrv_lookup_bs("foo", "base") be okay ("base" is a node-name that
> resides within "device"'s chain) or an error ("base" resolves to a
> different bs than "device")?  Again, all the more reason to have a
> common function decide the semantics we want, then all other clients
> automatically pick up on those semantics.
>
> > 
> > The preferred and recommended method now of using block-commit
> > is to specify the BDS to operate on via their node-name arguments.
> > 
> > This also adds an explicit check that base_bs is in the chain of
> > top_bs, because if they are referenced by their unique node-name
> > arguments, the discovery process does not intrinsically verify
> > they are in the same chain.
> 
> I like this addition.
> 
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  blockdev.c       | 35 +++++++++++++++++++++++++++++++++--
> >  qapi-schema.json | 35 ++++++++++++++++++++++++++---------
> >  qmp-commands.hx  | 29 ++++++++++++++++++++++-------
> >  3 files changed, 81 insertions(+), 18 deletions(-)
> > 
> 
> 
> 
> >  
> > -    if (top) {
> > +    /* Find the 'top' image file for the commit */
> > +    if (has_top_node_name) {
> > +        top_bs = bdrv_lookup_bs(NULL, top_node_name, &local_err);
> 
> Hmm.  Shouldn't you ALSO verify that 'top_bs' belongs to the chain owned
> by 'device'?  Later on, you verify that 'base_bs' belongs to 'top_bs',
> but if I pass node names, those names could have found a top unrelated
> to 'device'; and base related to that top.
>

It is not needed - it is not relevant for the active commit case (the
current check will catch that, since top_bs == bs).  And for the
non-active commit case, commit_start() will return an error if
'top_bs' is not in 'bs', so the case of 'base_bs' or 'top_bs' not
being in 'device' is already covered.

> Maybe that gives more weight to my idea of possibly allowing
> bdrv_lookup_bs(device, top_node_name), where the lookup succeeds even
> when device and top_node_name resolve to different bs, but where
> top_node_name is a node in the chain of device.
> 
> > 
> > -    if (has_base && base) {
> > +    /* Find the 'base' image file for the commit */
> > +    if (has_base_node_name) {
> > +        base_bs = bdrv_lookup_bs(NULL, base_node_name, &local_err);
> > +        if (local_err) {
> > +            error_propagate(errp, local_err);
> > +            return;
> > +        }
> > +    } else if (has_base && base) {
> 
> Here, it doesn't matter whether you pass device or NULL for the device
> name...
> 

This conditional is just to determine if we are finding the backing
image referenced by the string "base" or just using the default
bottom-most base image.

> >  
> > +    /* Verify that 'base' is in the same chain as 'top' */
> > +    if (!bdrv_is_in_chain(top_bs, base_bs)) {
> > +        error_setg(errp, "'base' and 'top' are not in the same chain");
> > +        return;
> 
> ...because this check is still mandatory to prove that a user with chain:
> base <- sn1 <- sn2 <- active
> 
> is not calling 'device':'active', 'top-node-name':'sn1',
> 'base-node-name':'sn2' (all that bdrv_lookup_bs can do is verify that
> base-node-name belongs to device, not that it _also_ belongs to top_bs).
> 
> 
> > -# @base:   #optional The file name of the backing image to write data into.
> > -#                    If not specified, this is the deepest backing image
> > +# For 'base', either @base or @base-node-name may be set but not both. If
> > +# neither is specified, this is the deepest backing image
> 
> I like how you factored th is out up front...
> 
> >  #
> > -# @top:    #optional 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:          #optional The file name of the backing image to write data
> > +#                           into.
> > +#
> > +# @base-node-name #optional The name of the block driver state node of the
> > +#                           backing image to write data into.
> > +#                           (Since 2.1)
> > +#
> > +# For 'top', either @top or @top-node-name must be set but not both.
> 
> ...but here, you were not consistent.  I'd add "If neither is specified,
> this is the active image" here...
>

OK, thanks.

> > +#
> > +# @top:           #optional 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.
> 
> ...and drop the second sentence from here.
>

OK, thanks.

> > +#
> > +# @top-node-name: #optional The block driver state node name of the backing
> > +#                           image within the image chain, which contains the
> > +#                           topmost data to be committed down.
> > +#                           (Since 2.1)
> >  #
> >  #                    If top == base, that is an error.
> >  #                    If top == active, the job will not be completed by itself,
> > @@ -2120,17 +2135,19 @@
> >  #
> >  # Returns: Nothing on success
> >  #          If commit or stream is already active on this device, DeviceInUse
> > -#          If @device does not exist, DeviceNotFound
> > +#          If @device does not exist or cannot be determined, DeviceNotFound
> >  #          If image commit is not supported by this device, NotSupported
> > -#          If @base or @top is invalid, a generic error is returned
> > +#          If @base, @top, @base-node-name, @top-node-name invalid, GenericError
> >  #          If @speed is invalid, InvalidParameter
> > +#          If both @base and @base-node-name are specified, GenericError
> > +#          If both @top and @top-node-name are specified, GenericError
> 
> These last two are arguably sub-conditions of the earlier statement
> about @top and @top-node-name being invalid (since being invalid can
> include when both strings are used at once).  It wouldn't hurt my
> feelings to reduce the docs by two lines.
> 

OK.  I called it out explicitly, since it is currently different
behavior from the other functions.

> >  # Since: 1.3
> >  #
> >  ##
> >  { 'command': 'block-commit',
> > -  'data': { 'device': 'str', '*base': 'str', '*top': 'str',
> > -            '*speed': 'int' } }
> > +  'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str',
> > +            '*top': 'str', '*top-node-name': 'str', '*speed': 'int' } }
> 
> I'm okay with keeping 'device' mandatory, even though technically the
> use of a node-name could imply which device is intended.  That is, as
> long as the code correctly errors out when device and top-node-name
> don't resolve to the same chain :)
> 

I had thought about making it optional, but I do think it is better as
mandatory - it makes sure the user knows which chain they are
operating on, so it acts as a (small) check against user mistakes.

> 
> > +
> > +For 'top', either top or top-node-name must be set but not both.
> > +
> > +- "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.
> > +                    (json-string, optional)
> 
> Same blurb about hoisting the 'not specified => active' sentence to the
> common text, rather than leaving it in the 'top' text.
> 

OK, thanks.

[Qemu-devel] [PATCH 5/5] block: extend block-commit to accept a string for the backing file

[Jeff Cody]

On some image chains, QEMU may not always be able to resolve the
filenames properly, when updating the backing file of an image
after a block commit.

For instance, certain relative pathnames may fail, or drives may
have been specified originally by file descriptor (e.g. /dev/fd/???),
or a relative protocol pathname may have been used.

In these instances, QEMU may lack the information to be able to make
the correct choice, but the user or management layer most likely does
have that knowledge.

With this extension to the block-commit api, the user is able to change
the backing file of the overlay image as part of the block-commit
operation.

This allows the change to be 'safe', in the sense that if the attempt
to write the overlay image metadata fails, then the block-commit
operation returns failure, without disrupting the guest.

If the commit top is the active layer, then specifying the backing
file string will be treated as an error (there is no overlay image
to modify in that case).

If a backing file string is not specified in the command, the backing
file string to use is determined in the same manner as it was
previously.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c                   |  8 ++++++--
 block/commit.c            |  9 ++++++---
 blockdev.c                |  8 +++++++-
 include/block/block.h     |  3 ++-
 include/block/block_int.h |  3 ++-
 qapi-schema.json          | 18 ++++++++++++++++--
 qmp-commands.hx           | 14 +++++++++++++-
 7 files changed, 52 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index c4f77c2..7e63a1f 100644
--- a/block.c
+++ b/block.c
@@ -2547,12 +2547,15 @@ typedef struct BlkIntermediateStates {
  *
  * base <- active
  *
+ * If backing_file_str is non-NULL, it will be used when modifying top's
+ * overlay image metadata.
+ *
  * Error conditions:
  *  if active == top, that is considered an error
  *
  */
 int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
-                           BlockDriverState *base)
+                           BlockDriverState *base, const char *backing_file_str)
 {
     BlockDriverState *intermediate;
     BlockDriverState *base_bs = NULL;
@@ -2604,7 +2607,8 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
     }
 
     /* success - we can delete the intermediate states, and link top->base */
-    ret = bdrv_change_backing_file(new_top_bs, base_bs->filename,
+    backing_file_str = backing_file_str ? backing_file_str : base_bs->filename;
+    ret = bdrv_change_backing_file(new_top_bs, backing_file_str,
                                    base_bs->drv ? base_bs->drv->format_name : "");
     if (ret) {
         goto exit;
diff --git a/block/commit.c b/block/commit.c
index 5c09f44..91517d3 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -37,6 +37,7 @@ typedef struct CommitBlockJob {
     BlockdevOnError on_error;
     int base_flags;
     int orig_overlay_flags;
+    char *backing_file_str;
 } CommitBlockJob;
 
 static int coroutine_fn commit_populate(BlockDriverState *bs,
@@ -141,7 +142,7 @@ wait:
 
     if (!block_job_is_cancelled(&s->common) && sector_num == end) {
         /* success */
-        ret = bdrv_drop_intermediate(active, top, base);
+        ret = bdrv_drop_intermediate(active, top, base, s->backing_file_str);
     }
 
 exit_free_buf:
@@ -158,7 +159,7 @@ exit_restore_reopen:
     if (overlay_bs && s->orig_overlay_flags != bdrv_get_flags(overlay_bs)) {
         bdrv_reopen(overlay_bs, s->orig_overlay_flags, NULL);
     }
-
+    g_free(s->backing_file_str);
     block_job_completed(&s->common, ret);
 }
 
@@ -182,7 +183,7 @@ static const BlockJobDriver commit_job_driver = {
 void commit_start(BlockDriverState *bs, BlockDriverState *base,
                   BlockDriverState *top, int64_t speed,
                   BlockdevOnError on_error, BlockDriverCompletionFunc *cb,
-                  void *opaque, Error **errp)
+                  void *opaque, const char *backing_file_str, Error **errp)
 {
     CommitBlockJob *s;
     BlockReopenQueue *reopen_queue = NULL;
@@ -244,6 +245,8 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
     s->base_flags          = orig_base_flags;
     s->orig_overlay_flags  = orig_overlay_flags;
 
+    s->backing_file_str = g_strdup(backing_file_str);
+
     s->on_error = on_error;
     s->common.co = qemu_coroutine_create(commit_run);
 
diff --git a/blockdev.c b/blockdev.c
index d8cb396..c0c7867 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1872,6 +1872,7 @@ void qmp_block_commit(const char *device,
                       bool has_base_node_name, const char *base_node_name,
                       bool has_top, const char *top,
                       bool has_top_node_name, const char *top_node_name,
+                      bool has_backing_file, const char *backing_file,
                       bool has_speed, int64_t speed,
                       Error **errp)
 {
@@ -1951,11 +1952,16 @@ void qmp_block_commit(const char *device,
     }
 
     if (top_bs == bs) {
+        if (has_backing_file) {
+            error_setg(errp, "'backing-file' specified,"
+                             " but 'top' is the active layer");
+            return;
+        }
         commit_active_start(bs, base_bs, speed, on_error, block_job_cb,
                             bs, &local_err);
     } else {
         commit_start(bs, base_bs, top_bs, speed, on_error, block_job_cb, bs,
-                    &local_err);
+                     backing_file, &local_err);
     }
     if (local_err != NULL) {
         error_propagate(errp, local_err);
diff --git a/include/block/block.h b/include/block/block.h
index 283a6f3..e7aeca6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -266,7 +266,8 @@ int bdrv_change_backing_file(BlockDriverState *bs,
     const char *backing_file, const char *backing_fmt);
 void bdrv_register(BlockDriver *bdrv);
 int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
-                           BlockDriverState *base);
+                           BlockDriverState *base,
+                           const char *backing_file_str);
 BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
                                     BlockDriverState *bs);
 BlockDriverState *bdrv_find_base(BlockDriverState *bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 9ffcb69..8e7dce7 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -428,13 +428,14 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base,
  * @on_error: The action to take upon error.
  * @cb: Completion function for the job.
  * @opaque: Opaque pointer value passed to @cb.
+ * @backing_file_str: String to use as the backing file in @top's overlay
  * @errp: Error object.
  *
  */
 void commit_start(BlockDriverState *bs, BlockDriverState *base,
                  BlockDriverState *top, int64_t speed,
                  BlockdevOnError on_error, BlockDriverCompletionFunc *cb,
-                 void *opaque, Error **errp);
+                 void *opaque, const char *backing_file_str, Error **errp);
 /**
  * commit_active_start:
  * @bs: Active block device to be committed.
diff --git a/qapi-schema.json b/qapi-schema.json
index eddb2b8..ef775fe 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2118,6 +2118,18 @@
 #                           topmost data to be committed down.
 #                           (Since 2.1)
 #
+# @backing-file: #optional The backing file string to write into the overlay
+#                          image of 'top'.  If 'top' is the active layer,
+#                          specifying a backing file string is an error. This
+#                          backing file string is only written into the the
+#                          image file metadata - internal structures inside
+#                          QEMU are not updated,  and the string is not validated.
+#                          If not specified, QEMU will automatically determine
+#                          the backing file string to use.  Care should be taken
+#                          when specifying the string, to specify a valid filename
+#                          or protocol.
+#                          (Since 2.1)
+#
 #                    If top == base, that is an error.
 #                    If top == active, the job will not be completed by itself,
 #                    user needs to complete the job with the block-job-complete
@@ -2130,7 +2142,6 @@
 #                    size of the smaller top, you can safely truncate it
 #                    yourself once the commit operation successfully completes.
 #
-#
 # @speed:  #optional the maximum speed, in bytes per second
 #
 # Returns: Nothing on success
@@ -2141,13 +2152,16 @@
 #          If @speed is invalid, InvalidParameter
 #          If both @base and @base-node-name are specified, GenericError
 #          If both @top and @top-node-name are specified, GenericError
+#          If @top or @top-node-name is the active layer, and @backing-file is
+#             specified, GenericError
 #
 # Since: 1.3
 #
 ##
 { 'command': 'block-commit',
   'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str',
-            '*top': 'str', '*top-node-name': 'str', '*speed': 'int' } }
+            '*top': 'str', '*top-node-name': 'str', '*backing-file': 'str',
+            '*speed': 'int' } }
 
 ##
 # @drive-backup
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 2b9b1dc..e2c446f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -985,7 +985,7 @@ EQMP
 
     {
         .name       = "block-commit",
-        .args_type  = "device:B,base:s?,base-node-name:s?,top:s?,top-node-name:s?,speed:o?",
+        .args_type  = "device:B,base:s?,base-node-name:s?,top:s?,top-node-name:s?,backing-file:s?,speed:o?",
         .mhandler.cmd_new = qmp_marshal_input_block_commit,
     },
 
@@ -1021,6 +1021,18 @@ For 'top', either top or top-node-name must be set but not both.
                    topmost data to be committed down.
                    (json-string, optional) (Since 2.1)
 
+- "backing-file": The backing file string to write into the overlay
+                  image of 'top'.  If 'top' is the active layer,
+                  specifying a backing file string is an error. This
+                  backing file string is only written into the the
+                  image file metadata - internal structures inside
+                  QEMU are not updated,  and the string is not validated.
+                  If not specified, QEMU will automatically determine
+                  the backing file string to use.  Care should be taken
+                  when specifying the string, to specify a valid filename
+                  or protocol.
+                  (json-string, optional) (Since 2.1)
+
           If top == base, that is an error.
           If top == active, the job will not be completed by itself,
           user needs to complete the job with the block-job-complete
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 5/5] block: extend block-commit to accept a string for the backing file

[Benoît Canet]

The Wednesday 14 May 2014 à 23:20:19 (-0400), Jeff Cody wrote :
> On some image chains, QEMU may not always be able to resolve the
> filenames properly, when updating the backing file of an image
> after a block commit.
> 
> For instance, certain relative pathnames may fail, or drives may
> have been specified originally by file descriptor (e.g. /dev/fd/???),
> or a relative protocol pathname may have been used.
> 
> In these instances, QEMU may lack the information to be able to make
> the correct choice, but the user or management layer most likely does
> have that knowledge.
> 
> With this extension to the block-commit api, the user is able to change
> the backing file of the overlay image as part of the block-commit
> operation.
> 
> This allows the change to be 'safe', in the sense that if the attempt
> to write the overlay image metadata fails, then the block-commit
> operation returns failure, without disrupting the guest.
> 
> If the commit top is the active layer, then specifying the backing
> file string will be treated as an error (there is no overlay image
> to modify in that case).
> 
> If a backing file string is not specified in the command, the backing
> file string to use is determined in the same manner as it was
> previously.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c                   |  8 ++++++--
>  block/commit.c            |  9 ++++++---
>  blockdev.c                |  8 +++++++-
>  include/block/block.h     |  3 ++-
>  include/block/block_int.h |  3 ++-
>  qapi-schema.json          | 18 ++++++++++++++++--
>  qmp-commands.hx           | 14 +++++++++++++-
>  7 files changed, 52 insertions(+), 11 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c4f77c2..7e63a1f 100644
> --- a/block.c
> +++ b/block.c
> @@ -2547,12 +2547,15 @@ typedef struct BlkIntermediateStates {
>   *
>   * base <- active
>   *
> + * If backing_file_str is non-NULL, it will be used when modifying top's
> + * overlay image metadata.
> + *
>   * Error conditions:
>   *  if active == top, that is considered an error
>   *
>   */
>  int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
> -                           BlockDriverState *base)
> +                           BlockDriverState *base, const char *backing_file_str)
>  {
>      BlockDriverState *intermediate;
>      BlockDriverState *base_bs = NULL;
> @@ -2604,7 +2607,8 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
>      }
>  
>      /* success - we can delete the intermediate states, and link top->base */
> -    ret = bdrv_change_backing_file(new_top_bs, base_bs->filename,
> +    backing_file_str = backing_file_str ? backing_file_str : base_bs->filename;
> +    ret = bdrv_change_backing_file(new_top_bs, backing_file_str,
>                                     base_bs->drv ? base_bs->drv->format_name : "");
>      if (ret) {
>          goto exit;
> diff --git a/block/commit.c b/block/commit.c
> index 5c09f44..91517d3 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -37,6 +37,7 @@ typedef struct CommitBlockJob {
>      BlockdevOnError on_error;
>      int base_flags;
>      int orig_overlay_flags;
> +    char *backing_file_str;
>  } CommitBlockJob;
>  
>  static int coroutine_fn commit_populate(BlockDriverState *bs,
> @@ -141,7 +142,7 @@ wait:
>  
>      if (!block_job_is_cancelled(&s->common) && sector_num == end) {
>          /* success */
> -        ret = bdrv_drop_intermediate(active, top, base);
> +        ret = bdrv_drop_intermediate(active, top, base, s->backing_file_str);
>      }
>  
>  exit_free_buf:
> @@ -158,7 +159,7 @@ exit_restore_reopen:
>      if (overlay_bs && s->orig_overlay_flags != bdrv_get_flags(overlay_bs)) {
>          bdrv_reopen(overlay_bs, s->orig_overlay_flags, NULL);
>      }
> -
> +    g_free(s->backing_file_str);
>      block_job_completed(&s->common, ret);
>  }
>  
> @@ -182,7 +183,7 @@ static const BlockJobDriver commit_job_driver = {
>  void commit_start(BlockDriverState *bs, BlockDriverState *base,
>                    BlockDriverState *top, int64_t speed,
>                    BlockdevOnError on_error, BlockDriverCompletionFunc *cb,
> -                  void *opaque, Error **errp)
> +                  void *opaque, const char *backing_file_str, Error **errp)
>  {
>      CommitBlockJob *s;
>      BlockReopenQueue *reopen_queue = NULL;
> @@ -244,6 +245,8 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
>      s->base_flags          = orig_base_flags;
>      s->orig_overlay_flags  = orig_overlay_flags;
>  
> +    s->backing_file_str = g_strdup(backing_file_str);
> +
>      s->on_error = on_error;
>      s->common.co = qemu_coroutine_create(commit_run);
>  
> diff --git a/blockdev.c b/blockdev.c
> index d8cb396..c0c7867 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1872,6 +1872,7 @@ void qmp_block_commit(const char *device,
>                        bool has_base_node_name, const char *base_node_name,
>                        bool has_top, const char *top,
>                        bool has_top_node_name, const char *top_node_name,
> +                      bool has_backing_file, const char *backing_file,
>                        bool has_speed, int64_t speed,
>                        Error **errp)
>  {
> @@ -1951,11 +1952,16 @@ void qmp_block_commit(const char *device,
>      }
>  
>      if (top_bs == bs) {
> +        if (has_backing_file) {
> +            error_setg(errp, "'backing-file' specified,"
> +                             " but 'top' is the active layer");
> +            return;
> +        }
>          commit_active_start(bs, base_bs, speed, on_error, block_job_cb,
>                              bs, &local_err);
>      } else {
>          commit_start(bs, base_bs, top_bs, speed, on_error, block_job_cb, bs,
> -                    &local_err);
> +                     backing_file, &local_err);

I don't know QAPI well enough to be sure but are we certain that if 
has_backing_file == false then backing_file == NULL and not some
random pointer ?

If am thinking to add has_backing_file ? backing_file : NULL here.


>      }
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
> diff --git a/include/block/block.h b/include/block/block.h
> index 283a6f3..e7aeca6 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -266,7 +266,8 @@ int bdrv_change_backing_file(BlockDriverState *bs,
>      const char *backing_file, const char *backing_fmt);
>  void bdrv_register(BlockDriver *bdrv);
>  int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
> -                           BlockDriverState *base);
> +                           BlockDriverState *base,
> +                           const char *backing_file_str);
>  BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
>                                      BlockDriverState *bs);
>  BlockDriverState *bdrv_find_base(BlockDriverState *bs);
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 9ffcb69..8e7dce7 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -428,13 +428,14 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base,
>   * @on_error: The action to take upon error.
>   * @cb: Completion function for the job.
>   * @opaque: Opaque pointer value passed to @cb.
> + * @backing_file_str: String to use as the backing file in @top's overlay

Miss a final dot to be like the other lines.

>   * @errp: Error object.
>   *
>   */
>  void commit_start(BlockDriverState *bs, BlockDriverState *base,
>                   BlockDriverState *top, int64_t speed,
>                   BlockdevOnError on_error, BlockDriverCompletionFunc *cb,
> -                 void *opaque, Error **errp);
> +                 void *opaque, const char *backing_file_str, Error **errp);
>  /**
>   * commit_active_start:
>   * @bs: Active block device to be committed.
> diff --git a/qapi-schema.json b/qapi-schema.json
> index eddb2b8..ef775fe 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2118,6 +2118,18 @@
>  #                           topmost data to be committed down.
>  #                           (Since 2.1)
>  #
> +# @backing-file: #optional The backing file string to write into the overlay
> +#                          image of 'top'.  If 'top' is the active layer,
> +#                          specifying a backing file string is an error. This
> +#                          backing file string is only written into the the
> +#                          image file metadata - internal structures inside
> +#                          QEMU are not updated,  and the string is not validated.
> +#                          If not specified, QEMU will automatically determine
> +#                          the backing file string to use.  Care should be taken
> +#                          when specifying the string, to specify a valid filename
> +#                          or protocol.
> +#                          (Since 2.1)
> +#
>  #                    If top == base, that is an error.
>  #                    If top == active, the job will not be completed by itself,
>  #                    user needs to complete the job with the block-job-complete
> @@ -2130,7 +2142,6 @@
>  #                    size of the smaller top, you can safely truncate it
>  #                    yourself once the commit operation successfully completes.
>  #
> -#

Spurious line removal.

>  # @speed:  #optional the maximum speed, in bytes per second
>  #
>  # Returns: Nothing on success
> @@ -2141,13 +2152,16 @@
>  #          If @speed is invalid, InvalidParameter
>  #          If both @base and @base-node-name are specified, GenericError
>  #          If both @top and @top-node-name are specified, GenericError
> +#          If @top or @top-node-name is the active layer, and @backing-file is
> +#             specified, GenericError
>  #
>  # Since: 1.3
>  #
>  ##
>  { 'command': 'block-commit',
>    'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str',
> -            '*top': 'str', '*top-node-name': 'str', '*speed': 'int' } }
> +            '*top': 'str', '*top-node-name': 'str', '*backing-file': 'str',
> +            '*speed': 'int' } }
>  
>  ##
>  # @drive-backup
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 2b9b1dc..e2c446f 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -985,7 +985,7 @@ EQMP
>  
>      {
>          .name       = "block-commit",
> -        .args_type  = "device:B,base:s?,base-node-name:s?,top:s?,top-node-name:s?,speed:o?",
> +        .args_type  = "device:B,base:s?,base-node-name:s?,top:s?,top-node-name:s?,backing-file:s?,speed:o?",
>          .mhandler.cmd_new = qmp_marshal_input_block_commit,
>      },
>  
> @@ -1021,6 +1021,18 @@ For 'top', either top or top-node-name must be set but not both.
>                     topmost data to be committed down.
>                     (json-string, optional) (Since 2.1)
>  
> +- "backing-file": The backing file string to write into the overlay
> +                  image of 'top'.  If 'top' is the active layer,
> +                  specifying a backing file string is an error. This
> +                  backing file string is only written into the the
> +                  image file metadata - internal structures inside
> +                  QEMU are not updated,  and the string is not validated.
> +                  If not specified, QEMU will automatically determine
> +                  the backing file string to use.  Care should be taken
> +                  when specifying the string, to specify a valid filename
> +                  or protocol.
> +                  (json-string, optional) (Since 2.1)
> +
>            If top == base, that is an error.
>            If top == active, the job will not be completed by itself,
>            user needs to complete the job with the block-job-complete
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 5/5] block: extend block-commit to accept a string for the backing file

[Eric Blake]

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

On 05/15/2014 06:26 AM, Benoît Canet wrote:
> The Wednesday 14 May 2014 à 23:20:19 (-0400), Jeff Cody wrote :
>> On some image chains, QEMU may not always be able to resolve the
>> filenames properly, when updating the backing file of an image
>> after a block commit.
>>

>>      } else {
>>          commit_start(bs, base_bs, top_bs, speed, on_error, block_job_cb, bs,
>> -                    &local_err);
>> +                     backing_file, &local_err);
> 
> I don't know QAPI well enough to be sure but are we certain that if 
> has_backing_file == false then backing_file == NULL and not some
> random pointer ?
> 
> If am thinking to add has_backing_file ? backing_file : NULL here.

We are moving towards having qapi guarantee sane defaults for FOO when
has_FOO is false; but aren't there yet.  You are correct that this needs
to guarantee that we aren't passing random memory.

-- 
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 5/5] block: extend block-commit to accept a string for the backing file

[Jeff Cody]

On Thu, May 15, 2014 at 06:57:11AM -0600, Eric Blake wrote:
> On 05/15/2014 06:26 AM, Benoît Canet wrote:
> > The Wednesday 14 May 2014 à 23:20:19 (-0400), Jeff Cody wrote :
> >> On some image chains, QEMU may not always be able to resolve the
> >> filenames properly, when updating the backing file of an image
> >> after a block commit.
> >>
> 
> >>      } else {
> >>          commit_start(bs, base_bs, top_bs, speed, on_error, block_job_cb, bs,
> >> -                    &local_err);
> >> +                     backing_file, &local_err);
> > 
> > I don't know QAPI well enough to be sure but are we certain that if 
> > has_backing_file == false then backing_file == NULL and not some
> > random pointer ?
> > 
> > If am thinking to add has_backing_file ? backing_file : NULL here.
> 
> We are moving towards having qapi guarantee sane defaults for FOO when
> has_FOO is false; but aren't there yet.  You are correct that this needs
> to guarantee that we aren't passing random memory.
>

The QAPI code generator for the QMP input marshaller initializes all
pointers to NULL, and all bools to false.  If has_ is false, then the
associated pointer will also be NULL, so it is safe to just pass
backing_file.

Re: [Qemu-devel] [PATCH 5/5] block: extend block-commit to accept a string for the backing file

[Eric Blake]

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

On 05/15/2014 07:10 AM, Jeff Cody wrote:

>>
>> We are moving towards having qapi guarantee sane defaults for FOO when
>> has_FOO is false; but aren't there yet.  You are correct that this needs
>> to guarantee that we aren't passing random memory.
>>
> 
> The QAPI code generator for the QMP input marshaller initializes all
> pointers to NULL, and all bools to false.  If has_ is false, then the
> associated pointer will also be NULL, so it is safe to just pass
> backing_file.

It didn't used to, and we still haven't documented that as being a
reliable guarantee, nor does the testsuite cover that.  It's still safer
to not rely on that fact until we make it part of the contract of qapi.

-- 
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 5/5] block: extend block-commit to accept a string for the backing file

[Eric Blake]

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

On 05/14/2014 09:20 PM, Jeff Cody wrote:
> On some image chains, QEMU may not always be able to resolve the
> filenames properly, when updating the backing file of an image
> after a block commit.

Do we want to allow qemu to error out in situations where it might get
things wrong (such as if a gluster protocol is backed by a local
filename) - and mandate that the user supply a backing string in those
situations?  But this patch is a strict improvement even if you don't go
that far, because it gives libvirt the flexibility to shift the burden
of name generation up the stack rather than sticking qemu with that task.

Hmm - how will this be discoverable by libvirt? Maybe when libvirt is
doing the 'qemu -m none' probing, it can hotplug a device pointing to
/dev/null (libvirt _already_ does that to test if add-fd works), and
intentionally omit a node name.  If libvirt then queries the device, and
sees that the __qemu##000NNNN node-name was auto-assigned, then it can
be assumed that this qemu is new enough to provide node-names for ALL
operations (but that means this series is incomplete unless we add
node-name support to all remaining block commands, such as block-stream,
drive-mirror, and drive-backup).  This part is where I wonder if patch
1/5 should be rebased to be last in the series.

> 
> For instance, certain relative pathnames may fail, or drives may
> have been specified originally by file descriptor (e.g. /dev/fd/???),

I'd document this as /dev/fdset/???, which is the magic string QMP uses
with its add-fd command (/dev/fd/??? is platform-specific whether it
will work, /dev/fdset/??? is guaranteed to work in all builds of qemu).

> or a relative protocol pathname may have been used.
> 
> In these instances, QEMU may lack the information to be able to make
> the correct choice, but the user or management layer most likely does
> have that knowledge.
> 
> With this extension to the block-commit api, the user is able to change
> the backing file of the overlay image as part of the block-commit
> operation.
> 
> This allows the change to be 'safe', in the sense that if the attempt
> to write the overlay image metadata fails, then the block-commit
> operation returns failure, without disrupting the guest.
> 
> If the commit top is the active layer, then specifying the backing
> file string will be treated as an error (there is no overlay image
> to modify in that case).
> 
> If a backing file string is not specified in the command, the backing
> file string to use is determined in the same manner as it was
> previously.

In short, this new command option allows the equivalent of 'qemu-img
rebase -u' on a live image.  Definitely a needed functionality.

> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c                   |  8 ++++++--
>  block/commit.c            |  9 ++++++---
>  blockdev.c                |  8 +++++++-
>  include/block/block.h     |  3 ++-
>  include/block/block_int.h |  3 ++-
>  qapi-schema.json          | 18 ++++++++++++++++--
>  qmp-commands.hx           | 14 +++++++++++++-
>  7 files changed, 52 insertions(+), 11 deletions(-)
> 

>      } else {
>          commit_start(bs, base_bs, top_bs, speed, on_error, block_job_cb, bs,
> -                    &local_err);
> +                     backing_file, &local_err);

See the rest of the thread about using 'has_backing_file ? backing_file
: NULL' here,


> +# @backing-file: #optional The backing file string to write into the overlay
> +#                          image of 'top'.  If 'top' is the active layer,
> +#                          specifying a backing file string is an error. This
> +#                          backing file string is only written into the the
> +#                          image file metadata - internal structures inside
> +#                          QEMU are not updated,  and the string is not validated.
> +#                          If not specified, QEMU will automatically determine
> +#                          the backing file string to use.  Care should be taken

Maybe "If not specified, QEMU will automatically determine a backing
file string to use, or error out if there is no obvious choice", to
allow us flexibility in erroring out on corner cases such as mixing
gluster with local files.

-- 
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 5/5] block: extend block-commit to accept a string for the backing file

[Jeff Cody]

On Thu, May 15, 2014 at 10:06:58AM -0600, Eric Blake wrote:
> On 05/14/2014 09:20 PM, Jeff Cody wrote:
> > On some image chains, QEMU may not always be able to resolve the
> > filenames properly, when updating the backing file of an image
> > after a block commit.
> 
> Do we want to allow qemu to error out in situations where it might get
> things wrong (such as if a gluster protocol is backed by a local
> filename) - and mandate that the user supply a backing string in those
> situations?  But this patch is a strict improvement even if you don't go
> that far, because it gives libvirt the flexibility to shift the burden
> of name generation up the stack rather than sticking qemu with that task.
> 

Yes, I think it would be a good idea to change the language here to
allow us to do that as needed.  I like what you wrote at the end of
your comments.


> Hmm - how will this be discoverable by libvirt? Maybe when libvirt is
> doing the 'qemu -m none' probing, it can hotplug a device pointing to
> /dev/null (libvirt _already_ does that to test if add-fd works), and
> intentionally omit a node name.  If libvirt then queries the device, and
> sees that the __qemu##000NNNN node-name was auto-assigned, then it can
> be assumed that this qemu is new enough to provide node-names for ALL
> operations (but that means this series is incomplete unless we add
> node-name support to all remaining block commands, such as block-stream,
> drive-mirror, and drive-backup).  This part is where I wonder if patch
> 1/5 should be rebased to be last in the series.
> 

Ah... I had originally planned on submitting separate patches for each
of the block jobs, to make reviewing easier.  But your idea on how
libvirt can discover this is a good one, and would mandate changing
those commands all in one series to be effective.  So this series will
grow by a few patches.  :)

If libvirt is going to use the autogenerated string format for
decisions, we should also document the string format in the QAPI docs.

> > 
> > For instance, certain relative pathnames may fail, or drives may
> > have been specified originally by file descriptor (e.g. /dev/fd/???),
> 
> I'd document this as /dev/fdset/???, which is the magic string QMP uses
> with its add-fd command (/dev/fd/??? is platform-specific whether it
> will work, /dev/fdset/??? is guaranteed to work in all builds of qemu).
> 

Sure, best to be consistent.

> > or a relative protocol pathname may have been used.
> > 
> > In these instances, QEMU may lack the information to be able to make
> > the correct choice, but the user or management layer most likely does
> > have that knowledge.
> > 
> > With this extension to the block-commit api, the user is able to change
> > the backing file of the overlay image as part of the block-commit
> > operation.
> > 
> > This allows the change to be 'safe', in the sense that if the attempt
> > to write the overlay image metadata fails, then the block-commit
> > operation returns failure, without disrupting the guest.
> > 
> > If the commit top is the active layer, then specifying the backing
> > file string will be treated as an error (there is no overlay image
> > to modify in that case).
> > 
> > If a backing file string is not specified in the command, the backing
> > file string to use is determined in the same manner as it was
> > previously.
> 
> In short, this new command option allows the equivalent of 'qemu-img
> rebase -u' on a live image.  Definitely a needed functionality.
> 

Would it be useful to have a stand-alone QMP command to change the
backing-file, as well?  As this stands, it will only change the
backing file if you are also merging data down the chain. 

If you want/need the ability to do a true 'qemu-img rebase -u' on any
given image without other chain modification, that needs a new
command.

> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block.c                   |  8 ++++++--
> >  block/commit.c            |  9 ++++++---
> >  blockdev.c                |  8 +++++++-
> >  include/block/block.h     |  3 ++-
> >  include/block/block_int.h |  3 ++-
> >  qapi-schema.json          | 18 ++++++++++++++++--
> >  qmp-commands.hx           | 14 +++++++++++++-
> >  7 files changed, 52 insertions(+), 11 deletions(-)
> > 
> 
> >      } else {
> >          commit_start(bs, base_bs, top_bs, speed, on_error, block_job_cb, bs,
> > -                    &local_err);
> > +                     backing_file, &local_err);
> 
> See the rest of the thread about using 'has_backing_file ? backing_file
> : NULL' here,
> 
> 
> > +# @backing-file: #optional The backing file string to write into the overlay
> > +#                          image of 'top'.  If 'top' is the active layer,
> > +#                          specifying a backing file string is an error. This
> > +#                          backing file string is only written into the the
> > +#                          image file metadata - internal structures inside
> > +#                          QEMU are not updated,  and the string is not validated.
> > +#                          If not specified, QEMU will automatically determine
> > +#                          the backing file string to use.  Care should be taken
> 
> Maybe "If not specified, QEMU will automatically determine a backing
> file string to use, or error out if there is no obvious choice", to
> allow us flexibility in erroring out on corner cases such as mixing
> gluster with local files.
> 

+1

Re: [Qemu-devel] [PATCH 5/5] block: extend block-commit to accept a string for the backing file

[Eric Blake]

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

On 05/15/2014 12:22 PM, Jeff Cody wrote:
>> Hmm - how will this be discoverable by libvirt? Maybe when libvirt is
>> doing the 'qemu -m none' probing, it can hotplug a device pointing to
>> /dev/null (libvirt _already_ does that to test if add-fd works), and
>> intentionally omit a node name.  If libvirt then queries the device, and
>> sees that the __qemu##000NNNN node-name was auto-assigned, then it can
>> be assumed that this qemu is new enough to provide node-names for ALL
>> operations (but that means this series is incomplete unless we add
>> node-name support to all remaining block commands, such as block-stream,
>> drive-mirror, and drive-backup).  This part is where I wonder if patch
>> 1/5 should be rebased to be last in the series.
>>
> 
> Ah... I had originally planned on submitting separate patches for each
> of the block jobs, to make reviewing easier.  But your idea on how
> libvirt can discover this is a good one, and would mandate changing
> those commands all in one series to be effective.  So this series will
> grow by a few patches.  :)
> 
> If libvirt is going to use the autogenerated string format for
> decisions, we should also document the string format in the QAPI docs.

Adding a new command is much easier to probe for (a single
query-commands, which we are already using) than requiring a sequence of
operations (hotplug, then query to see if a name was assigned), and
probably even more direct (if the standalone command exists, then so
does the integrated use of setting backing names).


>>
>> In short, this new command option allows the equivalent of 'qemu-img
>> rebase -u' on a live image.  Definitely a needed functionality.
>>
> 
> Would it be useful to have a stand-alone QMP command to change the
> backing-file, as well?  As this stands, it will only change the
> backing file if you are also merging data down the chain. 
> 
> If you want/need the ability to do a true 'qemu-img rebase -u' on any
> given image without other chain modification, that needs a new
> command.

Yes, I think that's probably a wise idea to provide a dedicated command
for just changing the name recorded in a backing file.

-- 
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 --]