[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in block
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add |
Date: |
Wed, 16 Sep 2015 13:18:24 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Wen Congyang <address@hidden> writes:
> 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.
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);
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.
- [Qemu-block] [PATCH v3 1/5] support nbd driver in blockdev-add, (continued)
- [Qemu-block] [PATCH v3 1/5] support nbd driver in blockdev-add, Wen Congyang, 2015/09/10
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Markus Armbruster, 2015/09/14
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Eric Blake, 2015/09/14
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Wen Congyang, 2015/09/14
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Markus Armbruster, 2015/09/15
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Wen Congyang, 2015/09/15
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Markus Armbruster, 2015/09/15
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Wen Congyang, 2015/09/16
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Markus Armbruster, 2015/09/16
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Wen Congyang, 2015/09/16
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add,
Markus Armbruster <=
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Eric Blake, 2015/09/16
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Wen Congyang, 2015/09/16
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Wen Congyang, 2015/09/16
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Markus Armbruster, 2015/09/17
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Eric Blake, 2015/09/15
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Kevin Wolf, 2015/09/15
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Wen Congyang, 2015/09/14
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Wen Congyang, 2015/09/14
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Eric Blake, 2015/09/14
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Wen Congyang, 2015/09/14