diff mbox

[v6,for,2.1,01/10] block: Auto-generate node_names for each BDS entry

Message ID 526da734a5f3cffd2eb56accafdb4add38c75270.1403041699.git.jcody@redhat.com
State New
Headers show

Commit Message

Jeff Cody June 17, 2014, 9:53 p.m. UTC
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 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.

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

Comments

Benoît Canet June 18, 2014, 12:53 p.m. UTC | #1
The Tuesday 17 Jun 2014 à 17:53:49 (-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 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

Jeff can't we simply enforce the namespace separation with a check on the QDict
option content ?
This way we could be sure that the user can't input a node-name starting with
__qemu.

> 
> 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.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 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 43abe96..da32bb0 100644
> --- a/block.c
> +++ b/block.c
> @@ -843,12 +843,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.9.3
>
Jeff Cody June 18, 2014, 1:13 p.m. UTC | #2
On Wed, Jun 18, 2014 at 02:53:15PM +0200, Benoît Canet wrote:
> The Tuesday 17 Jun 2014 à 17:53:49 (-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 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
> 
> Jeff can't we simply enforce the namespace separation with a check on the QDict
> option content ?
> This way we could be sure that the user can't input a node-name starting with
> __qemu.
>

That still would not stop a user from trying to 'predict' or assuming
what a node name would be ("oh, it is the first drive, it is probably
__qemu##0000", etc...).  Having the combination of the incrementing
counter and the random string generation guarantees 2 things: it will
always be unique in a qemu session, and it is not predictable by the
user.  The "__qemu##" just helps to visually identify it as a qemu
generated.

Although if you are strictly concerned about namespace confusion, we
could enforce the namespace as you suggest, so a user could not create
a node-name that would look like a qemu-generated node-name.  Even in
that case, I would still want to keep the sequential number + random
string.

> > 
> > 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.
> > 
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > 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 43abe96..da32bb0 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -843,12 +843,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.9.3
> >
Benoît Canet June 18, 2014, 1:31 p.m. UTC | #3
The Wednesday 18 Jun 2014 à 09:13:28 (-0400), Jeff Cody wrote :
> On Wed, Jun 18, 2014 at 02:53:15PM +0200, Benoît Canet wrote:
> > The Tuesday 17 Jun 2014 à 17:53:49 (-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 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
> > 
> > Jeff can't we simply enforce the namespace separation with a check on the QDict
> > option content ?
> > This way we could be sure that the user can't input a node-name starting with
> > __qemu.
> >
> 
> That still would not stop a user from trying to 'predict' or assuming
> what a node name would be ("oh, it is the first drive, it is probably
> __qemu##0000", etc...).  Having the combination of the incrementing
> counter and the random string generation guarantees 2 things: it will
> always be unique in a qemu session, and it is not predictable by the
> user.  The "__qemu##" just helps to visually identify it as a qemu
> generated.
> 
> Although if you are strictly concerned about namespace confusion, we
> could enforce the namespace as you suggest, so a user could not create
> a node-name that would look like a qemu-generated node-name.  Even in
> that case, I would still want to keep the sequential number + random
> string.

This way is fine for me.

> 
> > > 
> > > 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.
> > > 
> > > Reviewed-by: Eric Blake <eblake@redhat.com>
> > > 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 43abe96..da32bb0 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -843,12 +843,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.9.3
> > >
Stefan Hajnoczi June 19, 2014, 8:55 a.m. UTC | #4
On Tue, Jun 17, 2014 at 05:53:49PM -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 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.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)

Who is this feature for?

Human users: they'll need to  read through query-named-block-nodes
output to find the nodes they care about.  This is pretty cumbersome and
not human-friendly.

Management tools: parsing query-named-block-nodes isn't trivial since
the output can vary between QEMU versions (e.g. when we move I/O
throttling to a block driver node there will be new internal nodes).
Tools doing this should really use blockdev-add instead and assign their
own node names.

It seems like neither type of user will get much mileage out of this
feature.  Is it really necessary or did I miss a use case?

Stefan
Jeff Cody June 19, 2014, 12:30 p.m. UTC | #5
On Thu, Jun 19, 2014 at 04:55:02PM +0800, Stefan Hajnoczi wrote:
> On Tue, Jun 17, 2014 at 05:53:49PM -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 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.
> > 
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> Who is this feature for?
> 
> Human users: they'll need to  read through query-named-block-nodes
> output to find the nodes they care about.  This is pretty cumbersome and
> not human-friendly.
> 

Currently, that is how a human user would find the node-names.  That
doesn't mean there might not be a new interface later on, that is more
human friendly.

And while a human parsing query-named-block-nodes isn't fun, I think
it is easier than a human assigning node-names to a graph, so it is
more human-friendly than the current system.

> Management tools: parsing query-named-block-nodes isn't trivial since
> the output can vary between QEMU versions (e.g. when we move I/O
> throttling to a block driver node there will be new internal nodes).
> Tools doing this should really use blockdev-add instead and assign their
> own node names.

Libvirt (and OpenStack) is already testing with these patches, and my
impression from Eric is that parsing the output of
query-named-block-nodes was less work than assigning node-names in
libvirt.

Can you expand a bit on moving i/o throttle to a block, and creating
new internal nodes?

The generated node-names have the same life cycle as a specified
node-name; anything that would invalidate a generated node-name would
invalidate a specified node-name as well.

And if a QMP command is issued that would cause new nodes to be
assigned, I believe libvirt is aware that they need to perform a
query-named-block-nodes again.

> 
> It seems like neither type of user will get much mileage out of this
> feature.  Is it really necessary or did I miss a use case?
>

Strictly speaking, it isn't required.  But it makes sense for QEMU to
assign node-names to any unassigned node-names, because it does make
life easier for both humans and management software, and QEMU is the
only one that can always ensure that every BDS has a node-name.

It is also nice for QEMU; we can now in future versions assume that
every BDS will always have a node-name, regardless if it has been
assigned by the user or not.

And the usage of the node-names is strictly optional by the human or
management software user; neither is required to use the generated
node-names, and are feel to specify their own node-name.  A user
specified node-name will prevent an auto-generated one from being
assigned for that specific BDS.
Eric Blake June 19, 2014, 5:03 p.m. UTC | #6
On 06/19/2014 06:30 AM, Jeff Cody wrote:

>>> 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 resolving filenames
>>> (relative filenames, or file descriptors, symlinks, mounts, etc..) that
>>> qemu currently needs to deal with.
>>>

>> Who is this feature for?
>>
>> Human users: they'll need to  read through query-named-block-nodes
>> output to find the nodes they care about.  This is pretty cumbersome and
>> not human-friendly.
>>
> 
> Currently, that is how a human user would find the node-names.  That
> doesn't mean there might not be a new interface later on, that is more
> human friendly.
> 
> And while a human parsing query-named-block-nodes isn't fun, I think
> it is easier than a human assigning node-names to a graph, so it is
> more human-friendly than the current system.

I tend to agree here - it's easier to have a node name always present
(even if I have to look it up, and even if the lookup is a bit painful)
than it is to do experiments with block commands, and then realize only
after the fact that I forgot to name a node.  As a developer working on
a new algorithm in a management app for a particular sequence of chain
operations, I'd then know to fix up my code to add in explicit
node-naming earlier in the sequence anywhere that I had to use a
generated name later in the sequence, thanks to this visibility of a
node name through the human interfaces that I'm experimenting with.

> 
>> Management tools: parsing query-named-block-nodes isn't trivial since
>> the output can vary between QEMU versions (e.g. when we move I/O
>> throttling to a block driver node there will be new internal nodes).
>> Tools doing this should really use blockdev-add instead and assign their
>> own node names.
> 
> Libvirt (and OpenStack) is already testing with these patches, and my
> impression from Eric is that parsing the output of
> query-named-block-nodes was less work than assigning node-names in
> libvirt.

Actually (and some of this came from IRC) - libvirt isn't QUITE using
node-name yet.  As long as a backing chain is ALL local files, or ALL
gluster, then libvirt's current approach of using file names (whether
relative or absolute) has so far been sufficient to get everything done
we need for the current features being added to libvirt.  But yes,
definitely down the road, we plan on using node names.  Why? Because it
is impossible to have a mixed-mode chain (some local, some gluster) and
still have a sane way to refer to all elements in that chain without
node names.  It's just that it will be a long-term project that may take
several more months and refactorings in libvirt before we get there, so
we're not in a huge rush to have it supported directly in qemu 2.1.

> 
> Can you expand a bit on moving i/o throttle to a block, and creating
> new internal nodes?
> 
> The generated node-names have the same life cycle as a specified
> node-name; anything that would invalidate a generated node-name would
> invalidate a specified node-name as well.
> 
> And if a QMP command is issued that would cause new nodes to be
> assigned, I believe libvirt is aware that they need to perform a
> query-named-block-nodes again.

Yes - my goal with libvirt is to avoid generated names in all libvirt
interactions - but to have them as a nice fallback to show where I have
not yet met that goal.

> 
>>
>> It seems like neither type of user will get much mileage out of this
>> feature.  Is it really necessary or did I miss a use case?
>>
> 
> Strictly speaking, it isn't required.  But it makes sense for QEMU to
> assign node-names to any unassigned node-names, because it does make
> life easier for both humans and management software, and QEMU is the
> only one that can always ensure that every BDS has a node-name.
> 
> It is also nice for QEMU; we can now in future versions assume that
> every BDS will always have a node-name, regardless if it has been
> assigned by the user or not.

This is another nice aspect of the feature.  Coding around optional
names is trickier than coding around something guaranteed to exist.

> 
> And the usage of the node-names is strictly optional by the human or
> management software user; neither is required to use the generated
> node-names, and are feel to specify their own node-name.  A user
> specified node-name will prevent an auto-generated one from being
> assigned for that specific BDS.
> 

I'm still in favor of this patch, even if I hope to never take advantage
of it directly from libvirt.
Stefan Hajnoczi June 20, 2014, 4:24 a.m. UTC | #7
On Thu, Jun 19, 2014 at 08:30:19AM -0400, Jeff Cody wrote:
> On Thu, Jun 19, 2014 at 04:55:02PM +0800, Stefan Hajnoczi wrote:
> > On Tue, Jun 17, 2014 at 05:53:49PM -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 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.
> > > 
> > > Reviewed-by: Eric Blake <eblake@redhat.com>
> > > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > > ---
> > >  block.c | 16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > Who is this feature for?
> > 
> > Human users: they'll need to  read through query-named-block-nodes
> > output to find the nodes they care about.  This is pretty cumbersome and
> > not human-friendly.
> > 
> 
> Currently, that is how a human user would find the node-names.  That
> doesn't mean there might not be a new interface later on, that is more
> human friendly.
> 
> And while a human parsing query-named-block-nodes isn't fun, I think
> it is easier than a human assigning node-names to a graph, so it is
> more human-friendly than the current system.
> 
> > Management tools: parsing query-named-block-nodes isn't trivial since
> > the output can vary between QEMU versions (e.g. when we move I/O
> > throttling to a block driver node there will be new internal nodes).
> > Tools doing this should really use blockdev-add instead and assign their
> > own node names.
> 
> Libvirt (and OpenStack) is already testing with these patches, and my
> impression from Eric is that parsing the output of
> query-named-block-nodes was less work than assigning node-names in
> libvirt.
> 
> Can you expand a bit on moving i/o throttle to a block, and creating
> new internal nodes?

Currently I/O throttling is integrated into block.c.  This was done
because we had no clean way to add filter nodes (like I/O throttling,
compression, encryption, etc) on top of the format and protocol nodes.

Now we have almost reached the point where I/O throttling can be split
off into a BlockDriver.  When the feature is enabled an I/O throttling
node will be added into the graph.  When the feature is disabled, there
will be no I/O throttling node in the graph.

Stefan
Stefan Hajnoczi June 23, 2014, 12:41 p.m. UTC | #8
On Thu, Jun 19, 2014 at 08:30:19AM -0400, Jeff Cody wrote:
> On Thu, Jun 19, 2014 at 04:55:02PM +0800, Stefan Hajnoczi wrote:
> > On Tue, Jun 17, 2014 at 05:53:49PM -0400, Jeff Cody wrote:
> > It seems like neither type of user will get much mileage out of this
> > feature.  Is it really necessary or did I miss a use case?
> >
> 
> Strictly speaking, it isn't required.  But it makes sense for QEMU to
> assign node-names to any unassigned node-names, because it does make
> life easier for both humans and management software, and QEMU is the
> only one that can always ensure that every BDS has a node-name.
> 
> It is also nice for QEMU; we can now in future versions assume that
> every BDS will always have a node-name, regardless if it has been
> assigned by the user or not.
> 
> And the usage of the node-names is strictly optional by the human or
> management software user; neither is required to use the generated
> node-names, and are feel to specify their own node-name.  A user
> specified node-name will prevent an auto-generated one from being
> assigned for that specific BDS.

Thanks for the explanation.  I understand how auto-generated node-names
will be used a bit better now.  I think Eric and your arguments make
sense.

Stefan
diff mbox

Patch

diff --git a/block.c b/block.c
index 43abe96..da32bb0 100644
--- a/block.c
+++ b/block.c
@@ -843,12 +843,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 */