From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34837) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zbfrh-0004kx-L2 for qemu-devel@nongnu.org; Mon, 14 Sep 2015 22:21:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zbfrg-00080a-C5 for qemu-devel@nongnu.org; Mon, 14 Sep 2015 22:21:01 -0400 References: <1441878905-5272-1-git-send-email-wency@cn.fujitsu.com> <1441878905-5272-2-git-send-email-wency@cn.fujitsu.com> <87r3m1krdl.fsf@blackfin.pond.sub.org> <55F6EBF5.2090101@redhat.com> From: Wen Congyang Message-ID: <55F78059.4030309@cn.fujitsu.com> Date: Tue, 15 Sep 2015 10:20:09 +0800 MIME-Version: 1.0 In-Reply-To: <55F6EBF5.2090101@redhat.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 1/5] support nbd driver in blockdev-add List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , Markus Armbruster Cc: Kevin Wolf , Alberto Garcia , zhanghailiang , qemu block , Jiang Yunhong , Dong Eddie , qemu devel , "Dr. David Alan Gilbert" , Gonglei , Stefan Hajnoczi , Yang Hongyang On 09/14/2015 11:47 PM, Eric Blake wrote: > On 09/14/2015 08:27 AM, Markus Armbruster wrote: >> Wen Congyang 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 >>> Signed-off-by: zhanghailiang >>> Signed-off-by: Gonglei >>> --- >>> 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. >