Wen Congyang <we...@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 <we...@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 <we...@cn.fujitsu.com>
>>>> Signed-off-by: zhanghailiang <zhang.zhanghaili...@huawei.com>
>>>> Signed-off-by: Gonglei <arei.gong...@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' } }
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.
[...]
