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

[Jeff Cody]

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 <address@hidden>
> > ---
> >  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?