Re: [Qemu-devel] [PATCH v6 for 2.1 01/10] block: Auto-generate node_names for each BDS entry

["Benoît Canet" <benoit.canet@irqsave.net>]

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