From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50375) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WkuuT-0002w3-Bm for qemu-devel@nongnu.org; Thu, 15 May 2014 08:37:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WkuuN-0006gu-7L for qemu-devel@nongnu.org; Thu, 15 May 2014 08:37:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:18574) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WkuuM-0006gq-Ux for qemu-devel@nongnu.org; Thu, 15 May 2014 08:37:11 -0400 Date: Thu, 15 May 2014 08:37:06 -0400 From: Jeff Cody Message-ID: <20140515123706.GH8452@localhost.localdomain> References: <20140515115859.GF2812@irqsave.net> <20140515120614.GG8452@localhost.localdomain> <20140515123250.GI2812@irqsave.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140515123250.GI2812@irqsave.net> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?Beno=EEt?= Canet Cc: kwolf@redhat.com, pkrempa@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com On Thu, May 15, 2014 at 02:32:50PM +0200, Beno=EEt Canet wrote: > The Thursday 15 May 2014 =E0 08:06:14 (-0400), Jeff Cody wrote : > > On Thu, May 15, 2014 at 01:58:59PM +0200, Beno=EEt Canet wrote: > > > The Wednesday 14 May 2014 =E0 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. > > > >=20 > > > > If node_names are automatically generated when not specified, tha= t means > > > > that all block job operations can be done by reference to the uni= que > > > > node_name field. This eliminates ambiguity in filename pathing > > > > (relative filenames, or file descriptors, symlinks, mounts, etc..= ) that > > > > qemu currently needs to deal with. > > > >=20 > > > > If a node name is specified, then it will not be automatically > > > > generated for that BDS entry. > > > >=20 > > > > If it is automatically generated, it will be prefaced with "__qem= u##", > > > > followed by 8 characters of a unique number, followed by 8 random > > > > ASCII characters in the range of 'A-Z'. Some sample generated no= de-name > > > > strings: > > > > __qemu##00000000IAIYNXXR > > > > __qemu##00000002METXTRBQ > > > > __qemu##00000001FMBORDWG > > > >=20 > > > > The prefix is to aid in identifying it as a qemu-generated name, = the > > > > numeric portion is to guarantee uniqueness in a given qemu sessio= n, and > > > > the random characters are to further avoid any accidental collisi= ons > > > > with user-specified node-names. > > > >=20 > > > > Signed-off-by: Jeff Cody > > > > --- > > > > block.c | 16 +++++++++++++++- > > > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > >=20 > > > > 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; > > > > } > > > > =20 > > > > +#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]; > > >=20 > > > The room for the '\0' string termination seems to be missing: > > >=20 > > > char gen_node_name[GEN_NODE_NAME_MAX_LEN + 1]; > > > > >=20 > > The array includes room for it, note the use of 'sizeof()': > > #define GEN_NODE_NAME_MAX_LEN (sizeof(GEN_NODE_NAME_PREFIX) + 8= + 8) > >=20 > > sizeof() includes the '\0' in the length, while strlen() does not; > > e.g.: > > sizeof("four") =3D 5 > > strlen("four") =3D 4 > >=20 > > > > + static uint32_t counter; /* simple counter to guarantee uniq= ueness */ > > > > + > > > > + /* 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 =3D strlen(gen_node_name); > > > > + while (len < GEN_NODE_NAME_MAX_LEN - 1) { > > > > + gen_node_name[len++] =3D g_random_int_range('A', 'Z'= ); > > > > + } > > >=20 > > > Is this code generating only 7 random chars instead of 8 ? > > >=20 > >=20 > > 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') > >=20 > > > > + gen_node_name[GEN_NODE_NAME_MAX_LEN - 1] =3D '\0'; > > >=20 > > > Could be: > > > gen_node_name[GEN_NODE_NAME_MAX_LEN] =3D '\0'; > > > if the array is properly declared. > > > > >=20 > > That would go over the array bounds by 1. >=20 > 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. > >=20 > > > > + node_name =3D gen_node_name; > > > > } > > > > =20 > > > > /* empty string node name is invalid */ > > > > --=20 > > > > 1.8.3.1 > > > >=20