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

[Wen Congyang <wency@cn.fujitsu.com>]

On 09/15/2015 03:37 PM, Markus Armbruster wrote:
> Wen Congyang <wency@cn.fujitsu.com> writes:
> 
>> On 09/14/2015 11:47 PM, Eric Blake wrote:
>>> On 09/14/2015 08:27 AM, Markus Armbruster wrote:
>>>> Wen Congyang <wency@cn.fujitsu.com> 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 <wency@cn.fujitsu.com>
>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>>>> ---
>>>>>  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):
>>
>> Do you mean that: there is no need to support filename?
> 
> Rule of thumb: if the QMP command handler needs to parse a string
> argument, that argument should be a complex QAPI type instead.
> 
> Example: @filename needs to be parsed into its components, either
> 
>     * protocol unix, socket path, export name, or
>     * protocol tcp, host, port, export name
> 
> Since there's an either/or, the complex QAPI type should be a union.
> 
>>> { 'enum': 'NBDTransport', 'data': ['unix', 'tcp', 'udp'] }
>>
>> NBD only uses tcp, it doesn't support udp.
>>
>>> { 'union': 'BlockdevOptionsNBD',
>>>   'base': { 'transport': 'NBDTransport', 'export': 'str' },
>>>   'discriminator': 'transport',
>>>   'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }
>>> { 'struct': 'NBDUnix', 'data': { 'socket': 'str' } }
>>
>> unix socket needs a path, and I think we can use UnixSocketAddress here.
> 
> Yes, we should try to reuse common types like SocketAddress,
> InetSocketAddress, UnixSocketAddress.
> 
> Perhaps it could be as simple as
> 
>     { 'struct': 'BlockdevOptionsNBD',
>       'data': { 'addr: 'SocketAddress', 'export': 'str' } }

The problem is that: NBD doesn't use the fd.

Another question is: what key will we see in nbd_open()? "addr.host"
or "host"?

Thanks
Wen Congyang

> 
> Eric, what do you think?
> 
>>> { 'struct': 'NBDInet', 'data': { 'host': 'str', '*port': 'int',
>>>      '*ipv4': 'bool', '*ipv6': 'bool' } }
>>
>> Thanks for the above, and I will try it.
> 
> [...]
> .
>