From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50870) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZblCF-0001c4-Nj for qemu-devel@nongnu.org; Tue, 15 Sep 2015 04:02:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZblCC-0001AT-1I for qemu-devel@nongnu.org; Tue, 15 Sep 2015 04:02:35 -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> <55F776CC.3050601@cn.fujitsu.com> <87fv2g876a.fsf@blackfin.pond.sub.org> From: Wen Congyang Message-ID: <55F7D06D.1080205@cn.fujitsu.com> Date: Tue, 15 Sep 2015 16:01:49 +0800 MIME-Version: 1.0 In-Reply-To: <87fv2g876a.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset="windows-1252" 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: 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/15/2015 03:37 PM, Markus Armbruster wrote: > Wen Congyang writes: > >> 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): >> >> 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. Another question is: what key will we see in nbd_open()? "addr.host" or "host"? 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. > > [...] > . >