From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43561) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W1aSU-0002oe-Gr for qemu-devel@nongnu.org; Fri, 10 Jan 2014 06:41:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W1aSO-00006K-HR for qemu-devel@nongnu.org; Fri, 10 Jan 2014 06:41:02 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37958) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W1aSO-00006D-9R for qemu-devel@nongnu.org; Fri, 10 Jan 2014 06:40:56 -0500 Date: Fri, 10 Jan 2014 12:40:42 +0100 From: Kevin Wolf Message-ID: <20140110114042.GB4276@dhcp-200-207.str.redhat.com> References: <1388062130-21126-1-git-send-email-pl@kamp.de> <20140109141322.GB2862@dhcp-200-207.str.redhat.com> <52CEC998.4040409@kamp.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <52CEC998.4040409@kamp.de> Subject: Re: [Qemu-devel] [PATCHv5] block: add native support for NFS List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Lieven Cc: famz@redhat.com, ronniesahlberg@gmail.com, qemu-devel@nongnu.org, owasserm@redhat.com, stefanha@redhat.com, pbonzini@redhat.com Am 09.01.2014 um 17:08 hat Peter Lieven geschrieben: > Am 09.01.2014 15:13, schrieb Kevin Wolf: > > Am 26.12.2013 um 13:48 hat Peter Lieven geschrieben: > >> v4->v5: > >> - disussed with Ronnie and decided to move URL + Paramter parsing to LibNFS. > >> This allows for URL parameter processing directly in LibNFS without altering > >> the qemu NFS block driver. This bumps the version requirement for LibNFS > >> to 1.9.0 though. > > Considering that we'll likely want to add new options in the future, I'm > > not sure if this is a good idea. This means that struct nfs_url will > > change, and if qemu isn't updated, it might not even notice that some > > option was requested in a new field that it doesn't know and provide, > > so it will silently ignore it. Or if we have a qemu built against an > > older libnfs, this must be marked as an incompatible ABI change, so it > > can't run at all with the newer libnfs version. > > Maybe we are talking about differnt things here. The paramteres/options > we were talking about are options to libnfs not to qemu. This could be > e.g. the uid or the protocol version. This is nothing qemu has to care about. > The nfs_url struct is likely not to change and even if it would change > I see no problem as long as we do only extend the struct and do not change > to position of the server, export and file. The problem is that qemu doesn't treat nfsurl as a black box. It calls the libnfs function to parse a URL into the struct, and then it accesses the individual fields of struct nfsurl to pass them to several libnfs functions. If struct nfsurl is extended, qemu needs to learn to feed the new fields to libnfs functions, otherwise they'll be silently ignored. Just think it through what happens after adding a new option with the combinations of an old qemu binary run with a new libnfs, and a new qemu binary run with the old libnfs. You'll come to the conclusion that the ABI is incompatible both ways. > >> +static QemuOptsList runtime_opts = { > >> + .name = "nfs", > >> + .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head), > >> + .desc = { > >> + { > >> + .name = "filename", > >> + .type = QEMU_OPT_STRING, > >> + .help = "URL to the NFS file", > >> + }, > >> + { /* end of list */ } > >> + }, > >> +}; > > I think this is the point where I disagree. First of all, if it's a URL, > > call it "url" instead of "filename". But more importantly, a URL encodes > > options in a string instead of structured options that can be set > > separately. > Thats exactly what I meant above: The options are encoded in the > string, e.g. > > nfs://server/export/file?uid=1000&gid=1000 > > Thats what libnfs expects. But all the options are not important for qemu. This means that we only expose one string option to QMP clients. There is no way for them to figure out which options are valid, even with a QAPI schema. This will be a real problem for libvirt support for libnfs. It also means that you can't override single options of a libnfs backing file on the command line, though I suppose that is a minor use case. > "filename" was copied from block/iscsi.c. The semantic is exactly > the same. It accepts an URL and all the paramter parsing is > done inside libiscsi with iscsi_parse_url_full. Yup, and the part that you didn't copy is: /* TODO Convert to fine grained options */ The iscsi block driver is still using the legacy interface and pending conversion. Doesn't mean that this is a reason for a new block driver to not do it right from the beginning. Kevin