[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: |
Tue, 15 Sep 2015 10:20:09 +0800 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 |
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):
>
> { 'enum': 'NBDTransport', 'data': ['unix', 'tcp', 'udp'] }
> { 'union': 'BlockdevOptionsNBD',
> 'base': { 'transport': 'NBDTransport', 'export': 'str' },
> 'discriminator': 'transport',
> 'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }
Building fails:
GEN qmp-commands.h
In file included from /work/src/qemu/qapi-schema.json:9:
In file included from /work/src/qemu/qapi/block.json:6:
/work/src/qemu/qapi/block-core.json:1844: Flat union 'BlockdevOptionsNBD' must
have a string base field
Makefile:286: recipe for target 'qmp-commands.h' failed
make: *** [qmp-commands.h] Error 1
What about this:
{ 'struct': 'BlockdevOptionsNBDBase',
'data': { 'transport': 'NBDTransport', 'export': 'str' } }
{ 'union': 'BlockdevOptionsNBD',
'base': 'BlockdevOptionsNBDBase',
'discriminator': 'transport',
'data': { 'unix': 'NBDUnix', 'tcp': 'NBDInet', 'udp': 'NBDInet' } }
Thanks
Wen Congyang
> { 'struct': 'NBDUnix', 'data': { 'socket': 'str' } }
> { 'struct': 'NBDInet', 'data': { 'host': 'str', '*port': 'int',
> '*ipv4': 'bool', '*ipv6': 'bool' } }
>
>
>> I'm afraid this doesn't address Eric's review of your v2.
>
> Agreed; we still don't have the right interface.
>
>
>> Eric further observed that support for the nbd+unix transport was
>> missing, and suggested to have a union type combining the various
>> transports.
>
> And I just spelled out above what that should look like.
>
>>
>> If we decide separate types for single port and port ranges aren't
>> worthwhile, you can simply use SocketAddress where your v2 used
>> InetSocketAddress.
>
> I'm not sure if my 'NBDInet' above makes any more sense than reusing
> 'SocketAddress' (and if we do reuse 'SocketAddress', we have the further
> question of whether to split out socket ranges as a separate type so
> that SocketAddress becomes a single-port identity).
>
>>
>> Eric, how strongly do you feel about separating the two?
>
> I'm more worried about properly using a union type to represent unix vs.
> tcp, and less worried about SocketAddress vs. range types vs creating a
> new type (although in the long run, fixing ranges to be in a properly
> named type rather than re-inventing the struct across multiple
> transports is a good goal). But you are quite correct that I do not
> like the v3 proposal, because it encodes far too much information into a
> single '*filename':'str', which is not the qapi way.
>
- Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add, (continued)
- 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, 2015/09/16
- 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] [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 <=
- 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, 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, Eric Blake, 2015/09/15
- 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, Eric Blake, 2015/09/16
[Qemu-devel] [PATCH v3 5/5] hmp: add monitor command to add/remove a child, Wen Congyang, 2015/09/10
[Qemu-devel] [PATCH v3 4/5] qmp: add monitor command to add/remove a child, Wen Congyang, 2015/09/10