From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44869) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zbko4-0007yH-25 for qemu-devel@nongnu.org; Tue, 15 Sep 2015 03:37:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zbknz-0006Ni-9g for qemu-devel@nongnu.org; Tue, 15 Sep 2015 03:37:35 -0400 From: Markus Armbruster 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> Date: Tue, 15 Sep 2015 09:37:17 +0200 In-Reply-To: <55F776CC.3050601@cn.fujitsu.com> (Wen Congyang's message of "Tue, 15 Sep 2015 09:39:24 +0800") Message-ID: <87fv2g876a.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain 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: Wen Congyang Cc: Kevin Wolf , Alberto Garcia , zhanghailiang , qemu block , Jiang Yunhong , Dong Eddie , qemu devel , "Dr. David Alan Gilbert" , Gonglei , Stefan Hajnoczi , Yang Hongyang 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' } } 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. [...]