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

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

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
>