[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BD
From: |
Jeff Cody |
Subject: |
Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry |
Date: |
Thu, 15 May 2014 08:37:06 -0400 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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 <address@hidden>
> > > > ---
> > > > 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
> > > >
- [Qemu-devel] [PATCH 0/5] block: Modify block-commit to use node-names, Jeff Cody, 2014/05/14
- Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry, Eric Blake, 2014/05/15
- Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry, Eric Blake, 2014/05/15
- Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry, Jeff Cody, 2014/05/15
- Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry, Eric Blake, 2014/05/15
- Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry, Kevin Wolf, 2014/05/16
- Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry, Jeff Cody, 2014/05/16
- Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry, Eric Blake, 2014/05/16
- Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry, Kevin Wolf, 2014/05/16
[Qemu-devel] [PATCH 2/5] block: add helper function to determine if a BDS is in a chain, Jeff Cody, 2014/05/14