On 09/16/2015 07:18 PM, Markus Armbruster wrote:
> Wen Congyang <we...@cn.fujitsu.com> writes:
>
>> On 09/16/2015 04:21 PM, Markus Armbruster wrote:
>>> 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.
>
> Question still open.
It is just a matter of implementation. For other drivers, if the user specifies
an unknown option, we will report the error before calling qmp_blockdev_add().
If we allow the user specify fd, we may report the error in bdrv_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);
>
> Okay.
>
> Long term, I'd like to see us get rid of the conversions between
> QAPI-generated types and QDict / QemuOpts.
>
> Short term, you need to co-evolve the QDict-based code such as
> nbd_config() with the QAPI interfaces.
>
> Keeping the QAPI interface clean is more important than minimizing the
> implementation work, because we're free to mess with the implementation,
> but releasing a QAPI interface makes it ABI, so we better get it right.
> .
>
