Re: [Qemu-devel] [PATCHv5] block: add native support for NFS

[Kevin Wolf]

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