Wen Congyang <we...@cn.fujitsu.com> writes:
> On 09/15/2015 07:12 PM, Markus Armbruster wrote:
>> Wen Congyang <we...@cn.fujitsu.com> writes:
>>
>>> On 09/15/2015 03:37 PM, Markus Armbruster wrote:
>>>> 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' } }
>>>
>>> The problem is that: NBD doesn't use the fd.
>>
>> Is that fundamental, or just a matter of implementation?
Question still open.
>>> Another question is: what key will we see in nbd_open()? "addr.host"
>>> or "host"?
>>
>> As long as nbd_config() looks for "host" in the options QDict, we need
>> to put "host".
>
> Yes, we need "host" in nbd_config(), but we pass "addr.data.host" to it.
>
> How to avoid this problem?
Where is the code constructing the QDict?
