Message ID | 526da734a5f3cffd2eb56accafdb4add38c75270.1403041699.git.jcody@redhat.com |
---|---|
State | New |
Headers | show |
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. > > 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 >
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. > > > > 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 > >
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 > > >
On Tue, Jun 17, 2014 at 05:53:49PM -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 > > 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(-) Who is this feature for? Human users: they'll need to read through query-named-block-nodes output to find the nodes they care about. This is pretty cumbersome and not human-friendly. Management tools: parsing query-named-block-nodes isn't trivial since the output can vary between QEMU versions (e.g. when we move I/O throttling to a block driver node there will be new internal nodes). Tools doing this should really use blockdev-add instead and assign their own node names. It seems like neither type of user will get much mileage out of this feature. Is it really necessary or did I miss a use case? Stefan
On Thu, Jun 19, 2014 at 04:55:02PM +0800, Stefan Hajnoczi wrote: > On Tue, Jun 17, 2014 at 05:53:49PM -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 > > > > 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(-) > > Who is this feature for? > > Human users: they'll need to read through query-named-block-nodes > output to find the nodes they care about. This is pretty cumbersome and > not human-friendly. > Currently, that is how a human user would find the node-names. That doesn't mean there might not be a new interface later on, that is more human friendly. And while a human parsing query-named-block-nodes isn't fun, I think it is easier than a human assigning node-names to a graph, so it is more human-friendly than the current system. > Management tools: parsing query-named-block-nodes isn't trivial since > the output can vary between QEMU versions (e.g. when we move I/O > throttling to a block driver node there will be new internal nodes). > Tools doing this should really use blockdev-add instead and assign their > own node names. Libvirt (and OpenStack) is already testing with these patches, and my impression from Eric is that parsing the output of query-named-block-nodes was less work than assigning node-names in libvirt. Can you expand a bit on moving i/o throttle to a block, and creating new internal nodes? The generated node-names have the same life cycle as a specified node-name; anything that would invalidate a generated node-name would invalidate a specified node-name as well. And if a QMP command is issued that would cause new nodes to be assigned, I believe libvirt is aware that they need to perform a query-named-block-nodes again. > > It seems like neither type of user will get much mileage out of this > feature. Is it really necessary or did I miss a use case? > Strictly speaking, it isn't required. But it makes sense for QEMU to assign node-names to any unassigned node-names, because it does make life easier for both humans and management software, and QEMU is the only one that can always ensure that every BDS has a node-name. It is also nice for QEMU; we can now in future versions assume that every BDS will always have a node-name, regardless if it has been assigned by the user or not. And the usage of the node-names is strictly optional by the human or management software user; neither is required to use the generated node-names, and are feel to specify their own node-name. A user specified node-name will prevent an auto-generated one from being assigned for that specific BDS.
On 06/19/2014 06:30 AM, Jeff Cody wrote: >>> 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. >>> >> Who is this feature for? >> >> Human users: they'll need to read through query-named-block-nodes >> output to find the nodes they care about. This is pretty cumbersome and >> not human-friendly. >> > > Currently, that is how a human user would find the node-names. That > doesn't mean there might not be a new interface later on, that is more > human friendly. > > And while a human parsing query-named-block-nodes isn't fun, I think > it is easier than a human assigning node-names to a graph, so it is > more human-friendly than the current system. I tend to agree here - it's easier to have a node name always present (even if I have to look it up, and even if the lookup is a bit painful) than it is to do experiments with block commands, and then realize only after the fact that I forgot to name a node. As a developer working on a new algorithm in a management app for a particular sequence of chain operations, I'd then know to fix up my code to add in explicit node-naming earlier in the sequence anywhere that I had to use a generated name later in the sequence, thanks to this visibility of a node name through the human interfaces that I'm experimenting with. > >> Management tools: parsing query-named-block-nodes isn't trivial since >> the output can vary between QEMU versions (e.g. when we move I/O >> throttling to a block driver node there will be new internal nodes). >> Tools doing this should really use blockdev-add instead and assign their >> own node names. > > Libvirt (and OpenStack) is already testing with these patches, and my > impression from Eric is that parsing the output of > query-named-block-nodes was less work than assigning node-names in > libvirt. Actually (and some of this came from IRC) - libvirt isn't QUITE using node-name yet. As long as a backing chain is ALL local files, or ALL gluster, then libvirt's current approach of using file names (whether relative or absolute) has so far been sufficient to get everything done we need for the current features being added to libvirt. But yes, definitely down the road, we plan on using node names. Why? Because it is impossible to have a mixed-mode chain (some local, some gluster) and still have a sane way to refer to all elements in that chain without node names. It's just that it will be a long-term project that may take several more months and refactorings in libvirt before we get there, so we're not in a huge rush to have it supported directly in qemu 2.1. > > Can you expand a bit on moving i/o throttle to a block, and creating > new internal nodes? > > The generated node-names have the same life cycle as a specified > node-name; anything that would invalidate a generated node-name would > invalidate a specified node-name as well. > > And if a QMP command is issued that would cause new nodes to be > assigned, I believe libvirt is aware that they need to perform a > query-named-block-nodes again. Yes - my goal with libvirt is to avoid generated names in all libvirt interactions - but to have them as a nice fallback to show where I have not yet met that goal. > >> >> It seems like neither type of user will get much mileage out of this >> feature. Is it really necessary or did I miss a use case? >> > > Strictly speaking, it isn't required. But it makes sense for QEMU to > assign node-names to any unassigned node-names, because it does make > life easier for both humans and management software, and QEMU is the > only one that can always ensure that every BDS has a node-name. > > It is also nice for QEMU; we can now in future versions assume that > every BDS will always have a node-name, regardless if it has been > assigned by the user or not. This is another nice aspect of the feature. Coding around optional names is trickier than coding around something guaranteed to exist. > > And the usage of the node-names is strictly optional by the human or > management software user; neither is required to use the generated > node-names, and are feel to specify their own node-name. A user > specified node-name will prevent an auto-generated one from being > assigned for that specific BDS. > I'm still in favor of this patch, even if I hope to never take advantage of it directly from libvirt.
On Thu, Jun 19, 2014 at 08:30:19AM -0400, Jeff Cody wrote: > On Thu, Jun 19, 2014 at 04:55:02PM +0800, Stefan Hajnoczi wrote: > > On Tue, Jun 17, 2014 at 05:53:49PM -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 > > > > > > 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(-) > > > > Who is this feature for? > > > > Human users: they'll need to read through query-named-block-nodes > > output to find the nodes they care about. This is pretty cumbersome and > > not human-friendly. > > > > Currently, that is how a human user would find the node-names. That > doesn't mean there might not be a new interface later on, that is more > human friendly. > > And while a human parsing query-named-block-nodes isn't fun, I think > it is easier than a human assigning node-names to a graph, so it is > more human-friendly than the current system. > > > Management tools: parsing query-named-block-nodes isn't trivial since > > the output can vary between QEMU versions (e.g. when we move I/O > > throttling to a block driver node there will be new internal nodes). > > Tools doing this should really use blockdev-add instead and assign their > > own node names. > > Libvirt (and OpenStack) is already testing with these patches, and my > impression from Eric is that parsing the output of > query-named-block-nodes was less work than assigning node-names in > libvirt. > > Can you expand a bit on moving i/o throttle to a block, and creating > new internal nodes? Currently I/O throttling is integrated into block.c. This was done because we had no clean way to add filter nodes (like I/O throttling, compression, encryption, etc) on top of the format and protocol nodes. Now we have almost reached the point where I/O throttling can be split off into a BlockDriver. When the feature is enabled an I/O throttling node will be added into the graph. When the feature is disabled, there will be no I/O throttling node in the graph. Stefan
On Thu, Jun 19, 2014 at 08:30:19AM -0400, Jeff Cody wrote: > On Thu, Jun 19, 2014 at 04:55:02PM +0800, Stefan Hajnoczi wrote: > > On Tue, Jun 17, 2014 at 05:53:49PM -0400, Jeff Cody wrote: > > It seems like neither type of user will get much mileage out of this > > feature. Is it really necessary or did I miss a use case? > > > > Strictly speaking, it isn't required. But it makes sense for QEMU to > assign node-names to any unassigned node-names, because it does make > life easier for both humans and management software, and QEMU is the > only one that can always ensure that every BDS has a node-name. > > It is also nice for QEMU; we can now in future versions assume that > every BDS will always have a node-name, regardless if it has been > assigned by the user or not. > > And the usage of the node-names is strictly optional by the human or > management software user; neither is required to use the generated > node-names, and are feel to specify their own node-name. A user > specified node-name will prevent an auto-generated one from being > assigned for that specific BDS. Thanks for the explanation. I understand how auto-generated node-names will be used a bit better now. I think Eric and your arguments make sense. Stefan
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 */