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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2905 bytes --]

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.

> 
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]