From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33977) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZcAjR-0000em-Vl for qemu-devel@nongnu.org; Wed, 16 Sep 2015 07:18:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZcAjQ-00026g-Nt for qemu-devel@nongnu.org; Wed, 16 Sep 2015 07:18:33 -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> <87fv2g876a.fsf@blackfin.pond.sub.org> <55F7D06D.1080205@cn.fujitsu.com> <87mvwo3phm.fsf@blackfin.pond.sub.org> <55F9054F.1000001@cn.fujitsu.com> <87r3lyzsef.fsf@blackfin.pond.sub.org> <55F92745.6040904@cn.fujitsu.com> Date: Wed, 16 Sep 2015 13:18:24 +0200 In-Reply-To: <55F92745.6040904@cn.fujitsu.com> (Wen Congyang's message of "Wed, 16 Sep 2015 16:24:37 +0800") Message-ID: <87bnd2y5mn.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/16/2015 04:21 PM, Markus Armbruster wrote: >> Wen Congyang writes: >> >>> On 09/15/2015 07:12 PM, Markus Armbruster wrote: >>>> Wen Congyang writes: >>>> >>>>> 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. >>>> >>>> Is that fundamental, or just a matter of implementation? >> >> Question still open. Question still open. >>>>> 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? >> >> Where is the code constructing the QDict? > > The QDict is constructed in qmp_blockdev_add(): > visit_type_BlockdevOptions(qmp_output_get_visitor(ov), > &options, NULL, &local_err); > if (local_err) { > error_propagate(errp, local_err); > goto fail; > } > > obj = qmp_output_get_qobject(ov); > qdict = qobject_to_qdict(obj); > > qdict_flatten(qdict); Okay. Long term, I'd like to see us get rid of the conversions between QAPI-generated types and QDict / QemuOpts. Short term, you need to co-evolve the QDict-based code such as nbd_config() with the QAPI interfaces. Keeping the QAPI interface clean is more important than minimizing the implementation work, because we're free to mess with the implementation, but releasing a QAPI interface makes it ABI, so we better get it right.