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

[Wen Congyang]

On 09/16/2015 04:21 PM, Markus Armbruster wrote:
> Wen Congyang <address@hidden> writes:
> 
>> On 09/15/2015 07:12 PM, Markus Armbruster wrote:
>>> Wen Congyang <address@hidden> writes:
>>>
>>>> On 09/15/2015 03:37 PM, Markus Armbruster wrote:
>>>>> Wen Congyang <address@hidden> writes:
>>>>>
>>>>>> 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):
>>>>>>
>>>>>> 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?

The QDict is constructed in qmp_blockdev_add():
    visit_type_BlockdevOptions(qmp_output_get_visitor(ov),
                               &options, NULL, &local_err);
    if (local_err) {
        error_propagate(errp, local_err);
        goto fail;
    }

    obj = qmp_output_get_qobject(ov);
    qdict = qobject_to_qdict(obj);

    qdict_flatten(qdict);

Thanks
Wen Congyang
> .
>