[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add
From: |
Wen Congyang |
Subject: |
Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add |
Date: |
Wed, 16 Sep 2015 13:59:43 +0800 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 |
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?
>
>> 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?
Thanks
Wen Congyang
>
>> Thanks
>> Wen Congyang
>>
>>>
>>> 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.
>>>
>>> [...]
>>> .
>>>
> .
>
- [Qemu-devel] [PATCH v3 0/5] qapi: child add/delete support, Wen Congyang, 2015/09/10
- [Qemu-devel] [PATCH v3 2/5] Add new block driver interface to add/delete a BDS's child, Wen Congyang, 2015/09/10
- [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Wen Congyang, 2015/09/10
- Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Markus Armbruster, 2015/09/14
- Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Eric Blake, 2015/09/14
- Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Wen Congyang, 2015/09/14
- Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Markus Armbruster, 2015/09/15
- Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Wen Congyang, 2015/09/15
- Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Markus Armbruster, 2015/09/15
- Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add,
Wen Congyang <=
- Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Markus Armbruster, 2015/09/16
- Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Wen Congyang, 2015/09/16
- Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Markus Armbruster, 2015/09/16
- Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/5] support nbd driver in blockdev-add, Eric Blake, 2015/09/16
- Re: [Qemu-devel] [Qemu-block] [PATCH v3 1/5] support nbd driver in blockdev-add, Wen Congyang, 2015/09/16
- Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Wen Congyang, 2015/09/16
- Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Markus Armbruster, 2015/09/17
- Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Eric Blake, 2015/09/15
- Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Kevin Wolf, 2015/09/15
- Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, Wen Congyang, 2015/09/14