Re: [Qemu-block] [Qemu-devel] block/nfs: Fine grained runtime options in nfs

[Kevin Wolf]

Am 18.10.2016 um 15:14 hat Ashijeet Acharya geschrieben:
> On Tue, Oct 18, 2016 at 6:34 PM, Kevin Wolf <address@hidden> wrote:
> > Am 18.10.2016 um 14:46 hat Ashijeet Acharya geschrieben:
> >> On Tue, Oct 18, 2016 at 4:11 PM, Peter Lieven <address@hidden> wrote:
> >> > Am 17.10.2016 um 21:34 schrieb Ashijeet Acharya:
> >> >>
> >> >> On Tue, Oct 18, 2016 at 12:59 AM, Eric Blake <address@hidden> wrote:
> >> >>>
> >> >>> On 10/17/2016 01:00 PM, Ashijeet Acharya wrote:
> >> >>>
> >> >>>> One more relatively easy question though, will we include @port as an
> >> >>>> option in runtime_opts while converting NFS to use several
> >> >>>> runtime_opts? The reason I ask this because the uri syntax for NFS in
> >> >>>> QEMU looks like this:
> >> >>>>
> >> >>>>
> >> >>>> nfs://<host>/<export>/<filename>[?param=value[&param2=value2[&...]]]
> >> >>>
> >> >>> It's actually nfs://<host>[:port]/...
> >> >>>
> >> >>> so the URI syntax already supports port.
> >> >>
> >> >> But the commit message which added support for NFS had the uri which I
> >> >> mentioned above and the code for NFS does not make use of 'port'
> >> >> anywhere either, which is why I am a bit confused.
> >> >
> >> >
> >> > Hi Aschijeet,
> >> >
> >> > don't worry there is no port number when connecting to an NFS server.
> >> > The portmapper always listens on port 111. So theoretically we could
> >> > specifiy a port in the URL but it is ignored.
> >>
> >> So that means I will be including 'port' in runtime_opts and then just
> >> ignoring any value that comes through it?
> >
> > No, if there is nothing to configure there, leave it out. Adding an
> > option that doesn't do anything is not very useful.
> >
> Okay, understood.
> 
> So this is what my runtime_opts look like now:
> 
> static QemuOptsList runtime_opts = {
>     .name = "nfs",
>     .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
>     .desc = {
>         {
>             .name = "host",
>             .type = QEMU_OPT_STRING,
>             .help = "Host to connect to",
>         },
>         {
>             .name = "export",
>             .type = QEMU_OPT_STRING,
>             .help = "Name of the NFS export to open",
>         },
>         {
>             .name = "path",
>             .type = QEMU_OPT_STRING,
>             .help = "Path of the image on the host",
>         },

Does libnfs actually have separate export and path?

If I understand correctly, we currently split the URL at the last / in
the string. So is export the part before that and path the part after
it?

If so, does this mean that you can't currently access an image file in a
subdirectory of an NFS mount and adding the explicit options will fix
this?

>         {
>             .name = "uid",
>             .type = QEMU_OPT_NUMBER,
>             .help = "UID value to use when talking to the server",
>         },
>         {
>             .name = "gid",
>             .type = QEMU_OPT_NUMBER,
>             .help = "GID value to use when talking to the server",
>         },
>         {
>             .name = "tcp-syncnt",
>             .type = QEMU_OPT_NUMBER,
>             .help = "Number of SYNs to send during the session establish",
>         },
>         {
>             .name = "readahead",
>             .type = QEMU_OPT_NUMBER,
>             .help = "Set the readahead size in bytes",
>         },
>         {
>             .name = "pagecache",
>             .type = QEMU_OPT_NUMBER,
>             .help = "Set the pagecache size in bytes",
>         },
>         {
>             .name = "debug",
>             .type = QEMU_OPT_NUMBER,
>             .help = "Set the NFS debug level (max 2)",
>         },
>         { /* end of list */ }
>     },
> };
> 
> Please check if I have missed out on anything.

I don't see anything, but then I'm not an expert on NFS either. I hope
Peter can take a look.

Though maybe it's easier to see in the context of the full patch.

> I have successfully converted NFS block driver to use this set of
> runtime opts which I think is the required condition to add
> blockdev-add compatibility later. Also, since I do not have 'port' as
> a runtime option, I can directly add blockdev-add compatibility after
> this through qapi/block-core.json and will not have to go through the
> tricky method we are implementing for NBD and SSH as there will be no
> use of InetSocketAddress. Right?

Yes, InetSocketAddress is what makes things a bit tricky, and it doesn't
seem to be useful with the API we get from libnfs, so just directly
taking a host name should be okay. Then this one should be easier than
SSH.

Eric, do you agree, or do you think we should take into account that
libnfs might be extended one day to work on any socket?

Kevin