| Message ID | dd879b4363d89722045f538a67f09193135d4bdd.1400123059.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 >
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 <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]; > 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. > > + node_name = gen_node_name; > > } > > > > /* empty string node name is invalid */ > > -- > > 1.8.3.1 > >
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 <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]; > > > > 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. > > > > + node_name = gen_node_name; > > > } > > > > > > /* empty string node name is invalid */ > > > -- > > > 1.8.3.1 > > >
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 <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]; > > > > > > > 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 > > > >
On 05/14/2014 09:20 PM, 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 "pathing" sounds weird; a non-native speaker may get confused since it is not a real word. Maybe: 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. > > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > { > + 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); Why not just len = snprintf(), and avoid the strlen()? But that micro-optimization is not on the hot path, so: Reviewed-by: Eric Blake <eblake@redhat.com>
On 05/14/2014 09:20 PM, 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(-) This patch feels incomplete. We probably also ought to modify qapi-schema.json to make BlockDeviceInfo's output of 'node-name' by unconditional (as an output-only struct, changing from optional to mandatory is a safe change for back-compat API considerations); which implies further modifying code to get rid of has_node_name arguments in all places that generate BlockDeviceInfo details. See also my thoughts on patch 5/5 - can this patch be rebased to be LAST in the series, rather than first, so that it serves as a reliable witness that everything else related to block operations on node-names is complete?
On Thu, May 15, 2014 at 09:59:01AM -0600, Eric Blake wrote: > On 05/14/2014 09:20 PM, 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(-) > > This patch feels incomplete. We probably also ought to modify > qapi-schema.json to make BlockDeviceInfo's output of 'node-name' by > unconditional (as an output-only struct, changing from optional to > mandatory is a safe change for back-compat API considerations); which > implies further modifying code to get rid of has_node_name arguments in > all places that generate BlockDeviceInfo details. I think it should be a separate patch (but still in this series). Strictly speaking, this patch doesn't force the argument to be mandatory, the value just happens to always be populated now. > See also my thoughts on patch 5/5 - can this patch be rebased to be LAST > in the series, rather than first, so that it serves as a reliable > witness that everything else related to block operations on node-names > is complete? > How about this becomes the penultimate patch, and the patch to make BlockDeviceInfo's node-name field to be unconditional becomes the last patch? The only thing I don't like about moving this further back in the patch series is it makes the earlier patches untestable; I can't easily test the usage of the node-names for various intermediate BDS because they don't have node-names set. So that means I'll just need to rebase the patches prior to sending. So let me revise my above statement - how about this patch stays at the beginning, which makes development / testing easier, with the patch that modifies BlockDeviceInfo as the final (not including tests) patch?
On 05/15/2014 12:41 PM, Jeff Cody wrote: >>> 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(-) >> >> This patch feels incomplete. We probably also ought to modify >> qapi-schema.json to make BlockDeviceInfo's output of 'node-name' by >> unconditional (as an output-only struct, changing from optional to >> mandatory is a safe change for back-compat API considerations); which >> implies further modifying code to get rid of has_node_name arguments in >> all places that generate BlockDeviceInfo details. > > I think it should be a separate patch (but still in this series). > Strictly speaking, this patch doesn't force the argument to be > mandatory, the value just happens to always be populated now. True, keeping it as two patches is cleaner. > >> See also my thoughts on patch 5/5 - can this patch be rebased to be LAST >> in the series, rather than first, so that it serves as a reliable >> witness that everything else related to block operations on node-names >> is complete? >> > How about this becomes the penultimate patch, and the patch to make > BlockDeviceInfo's node-name field to be unconditional becomes the last > patch? Or, if we go the route of adding a new command for setting the backing name in isolation, then _that_ patch should be last, at which point leaving this patch early in the series no longer hurts, because it is no longer the witness that libvirt would be relying on. > > The only thing I don't like about moving this further back in the > patch series is it makes the earlier patches untestable; I can't > easily test the usage of the node-names for various intermediate BDS > because they don't have node-names set. So that means I'll just need > to rebase the patches prior to sending. So let me revise my above > statement - how about this patch stays at the beginning, which makes > development / testing easier, with the patch that modifies > BlockDeviceInfo as the final (not including tests) patch? Yes, keeping this patch first sounds reasonable after all. The patch modifying BlockDeviceInfo isn't introspectible (there's nothing libvirt can probe that says whether the QMP code switched from optional to mandatory - all libvirt can probe is whether a name gets generated - and that is THIS patch, not the BlockDeviceInfo patch). But adding a new command is easier to use than probing whether a name was generated. > >
Am 15.05.2014 um 20:41 hat Jeff Cody geschrieben: > The only thing I don't like about moving this further back in the > patch series is it makes the earlier patches untestable; I can't > easily test the usage of the node-names for various intermediate BDS > because they don't have node-names set. So that means I'll just need > to rebase the patches prior to sending. I don't quite follow. Can't you always manually assign node-names? This is how libvirt is supposed to use the interface. I'm not totally sure whether automatically generated node-names are a good idea, but I can see how they are useful with human monitor users which may not specify a node-name everywhere (I've used device_add without an ID often enough, only to find that I can't remove the device any more). We should just make sure that they are really only used by human users. Kevin
On Fri, May 16, 2014 at 11:39:27AM +0200, Kevin Wolf wrote: > Am 15.05.2014 um 20:41 hat Jeff Cody geschrieben: > > The only thing I don't like about moving this further back in the > > patch series is it makes the earlier patches untestable; I can't > > easily test the usage of the node-names for various intermediate BDS > > because they don't have node-names set. So that means I'll just need > > to rebase the patches prior to sending. > > I don't quite follow. Can't you always manually assign node-names? This > is how libvirt is supposed to use the interface. > How does libvirt assign node-names to all the backing images in a qcow2 chain, for example? > I'm not totally sure whether automatically generated node-names are > a good idea, but I can see how they are useful with human monitor users > which may not specify a node-name everywhere (I've used device_add > without an ID often enough, only to find that I can't remove the device > any more). We should just make sure that they are really only used by > human users. > I don't understand. What would be the downsides of having an automatic guaranteed unique id assigned to each BDS? And why restrict that to human users only? If you are worried about node-names potentially being undesired by the user / management layer for some reason, how about this: we add a drive option, to either enable or disable automatic node-name generation for a particular drive?
On 05/16/2014 05:35 AM, Jeff Cody wrote: > > How does libvirt assign node-names to all the backing images in a > qcow2 chain, for example? We have the ability to do both command-line and QMP hotplug addition of drives where you can set backing.file.node-name (or is it backing.node-name?) for any element in a backing chain; libvirt just needs to be taught to use it. > > >> I'm not totally sure whether automatically generated node-names are >> a good idea, but I can see how they are useful with human monitor users >> which may not specify a node-name everywhere (I've used device_add >> without an ID often enough, only to find that I can't remove the device >> any more). We should just make sure that they are really only used by >> human users. >> > > I don't understand. What would be the downsides of having an > automatic guaranteed unique id assigned to each BDS? And why > restrict that to human users only? I'm not seeing downsides. > > If you are worried about node-names potentially being undesired by the > user / management layer for some reason, how about this: we add a > drive option, to either enable or disable automatic node-name > generation for a particular drive? No, this is silly. If it defaults to off, then libvirt has to be taught to enable it - but if you are going to modify libvirt, you might as well modify it to set node names itself rather than to enable the option for automatic node names. If it defaults to on, no one has an incentive to disable it.
Am 16.05.2014 um 14:47 hat Eric Blake geschrieben: > On 05/16/2014 05:35 AM, Jeff Cody wrote: > > > > > How does libvirt assign node-names to all the backing images in a > > qcow2 chain, for example? > > We have the ability to do both command-line and QMP hotplug addition of > drives where you can set backing.file.node-name (or is it > backing.node-name?) for any element in a backing chain; libvirt just > needs to be taught to use it. backing.node-name and backing.file.node-name usually both exist, and they are different nodes (one is the qcow2 layer, the other on the raw-posix layer). Kevin
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]; + 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 */
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(-)