Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add

[Wen Congyang]

On 09/15/2015 10:20 AM, Wen Congyang wrote:
> On 09/14/2015 11:47 PM, Eric Blake wrote:
>> On 09/14/2015 08:27 AM, Markus Armbruster wrote:
>>> Wen Congyang <address@hidden> writes:
>>>
>>>> The NBD driver needs: filename, path or (host, port, exportname).
>>>> It checks which key exists and decides use unix or inet socket.
>>>> It doesn't recognize the key type, so we can't use union, and
>>>> can't reuse InetSocketAddress.
>>>>
>>>> Signed-off-by: Wen Congyang <address@hidden>
>>>> Signed-off-by: zhanghailiang <address@hidden>
>>>> Signed-off-by: Gonglei <address@hidden>
>>>> ---
>>>>  qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++--
>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>
>>
>>>>  ##
>>>> +# @BlockdevOptionsNBD
>>>> +#
>>>> +# Driver specific block device options for NBD
>>>> +#
>>>> +# @filename: #optional unix or inet path. The format is:
>>>> +#            unix: nbd+unix:///export?socket=path or
>>>> +#                  nbd:unix:path:exportname=export
>>>> +#            inet: nbd[+tcp]://host[:port]/export or
>>>> +#                  nbd:host[:port]:exportname=export
>>
>> Yuck. You are passing structured data through a single 'str', when you
>> SHOULD be passing it through structured JSON.  Just because we have a
>> filename shorthand for convenience does NOT mean that we want to expose
>> that convenience in QMP.  Instead, we really want the breakdown of the
>> pieces (here, using abbreviated syntax of an inline base, since there
>> are pending qapi patches that will allow it):
>>
>> { 'enum': 'NBDTransport', 'data': ['unix', 'tcp', 'udp'] }
>> { 'union': 'BlockdevOptionsNBD',
>>   'base': { 'transport': 'NBDTransport', 'export': 'str' },
>>   'discriminator': 'transport',
>>   'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }
> 
> Building fails:
>   GEN   qmp-commands.h
> In file included from /work/src/qemu/qapi-schema.json:9:
> In file included from /work/src/qemu/qapi/block.json:6:
> /work/src/qemu/qapi/block-core.json:1844: Flat union 'BlockdevOptionsNBD' 
> must have a string base field
> Makefile:286: recipe for target 'qmp-commands.h' failed
> make: *** [qmp-commands.h] Error 1
> 
> What about this:
> { 'struct': 'BlockdevOptionsNBDBase',
>   'data': {  'transport': 'NBDTransport', 'export': 'str' } }
> { 'union': 'BlockdevOptionsNBD',
>   'base': 'BlockdevOptionsNBDBase',
>   'discriminator': 'transport',
>   'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }

Another problem:
In file included from /work/src/qemu/qapi-schema.json:9:
In file included from /work/src/qemu/qapi/block.json:6:
/work/src/qemu/qapi/block-core.json:1866: Member 'nbd' of union 
'BlockdevOptions' cannot use union type 'BlockdevOptionsNBD'
Makefile:286: recipe for target 'qmp-commands.h' failed

Thanks
Wen Congyang

> 
> Thanks
> Wen Congyang
> 
>> { 'struct': 'NBDUnix', 'data': { 'socket': 'str' } }
>> { 'struct': 'NBDInet', 'data': { 'host': 'str', '*port': 'int',
>>      '*ipv4': 'bool', '*ipv6': 'bool' } }
>>
>>
>>> I'm afraid this doesn't address Eric's review of your v2.
>>
>> Agreed; we still don't have the right interface.
>>
>>
>>> Eric further observed that support for the nbd+unix transport was
>>> missing, and suggested to have a union type combining the various
>>> transports.
>>
>> And I just spelled out above what that should look like.
>>
>>>
>>> If we decide separate types for single port and port ranges aren't
>>> worthwhile, you can simply use SocketAddress where your v2 used
>>> InetSocketAddress.
>>
>> I'm not sure if my 'NBDInet' above makes any more sense than reusing
>> 'SocketAddress' (and if we do reuse 'SocketAddress', we have the further
>> question of whether to split out socket ranges as a separate type so
>> that SocketAddress becomes a single-port identity).
>>
>>>
>>> Eric, how strongly do you feel about separating the two?
>>
>> I'm more worried about properly using a union type to represent unix vs.
>> tcp, and less worried about SocketAddress vs. range types vs creating a
>> new type (although in the long run, fixing ranges to be in a properly
>> named type rather than re-inventing the struct across multiple
>> transports is a good goal).  But you are quite correct that I do not
>> like the v3 proposal, because it encodes far too much information into a
>> single '*filename':'str', which is not the qapi way.
>>
> 
> 
> .
>