On Fri, Dec 6, 2024 at 4:54 PM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote: > Here are some initial review comments to get the ball rolling. > On 12/6/24 5:54 AM, Roland Mainz wrote: > > Below (and also available at https://nrubsig.kpaste.net/b37) is a > > patch which adds support for nfs://-URLs in mount.nfs4, as alternative > > to the traditional hostname:/path+-o port=<tcp-port> notation. > > > > * Main advantages are: > > - Single-line notation with the familiar URL syntax, which includes > > hostname, path *AND* TCP port number (last one is a common generator > > of *PAIN* with ISPs) in ONE string > > - Support for non-ASCII mount points, e.g. paths with CJKV (Chinese, > > s/mount points/export paths OK > (When/if you need to repost, you should move this introductory text into > a cover letter.) What is a "cover letter" in this context ? git feature, or something else ? > > Japanese, ...) characters, which is typically a big problem if you try > > to transfer such mount point information across email/chat/clipboard > > etc., which tends to mangle such characters to death (e.g. > > transliteration, adding of ZWSP or just '?'). > > - URL parameters are supported, providing support for future extensions > > IMO, any support for URL parameters should be dropped from this > patch and then added later when we know what the parameters look > like. Generally, we avoid adding extra code until we have actual > use cases. Keeps things simple and reduces technical debt and dead > code. Originally we had much more URL parameters in the patch. After lots of debate I've cut that part down to the set (i.e. { "rw", "ro" } * { "0", "1" }, e.g. "ro=1", "ro=0" etc,) which is supported by ALL nfs://-URL implementations right now - which means if we revise the nfs://-URL RFC to make nfs://-URLs independent then we have to cover the existing "ro"/"rw" URL parameters anyway. Technically I can rip it out for now, but per URL RFC *any* unexpected URL parameter must be fatal, which in this case will break stuff. That's why I would prefer to keep this code, and it's also intended to demonstrate how to implement URL parameters correctly. Maybe split this off into a 2nd patch ? > > * Notes: > > - Similar support for nfs://-URLs exists in other NFSv4.* > > implementations, including Illumos, Windows ms-nfs41-client, > > sahlberg/libnfs, ... > > The key here is that this proposal is implementing a /standard/ > (RFC 2224). Right, I wasn't sure whether to quote it or not, because section 2 of that RFC quotes WebNFS, and some people are afraid of "WebNFS-will-come-back-from-the-grave-to-have-it's-revenge"... :-) ... but yes, I'll quote that. > > - This is NOT about WebNFS, this is only to use an URL representation > > to make the life of admins a LOT easier > > - Only absolute paths are supported > > This is actually a critical part of this proposal, IMO. > > RFC 2224 specifies two types of NFS URL: one that specifies an > absolute path, which is relative to the server's root FH, and > one that specifies a relative path, which is relative to the > server's PUB FH. > > You are adding support for only the absolute path style. This > means the URL is converted to string mount options by mount.nfs > and no code changes are needed in the kernel. There is no new > requirement for support of PUBFH. Right, I know. And the code will reject any relative URLs ($ grep -r -F 'Relative nfs://-URLs are not supported.' #) ... > I wonder how distributions will test the ability to mount > percent-escaped path names, though. Maybe not upstream's problem. It's more an issue to generate the URLs, but any URL-encoding-web page can do that. I also have http://svn.nrubsig.org/svn/people/gisburn/scripts/nfsurlconv/nfsurlconv.ksh and other utilities, but that would be a seperate patch series. DISCLAIMER: Yes, it's a ksh(93) script (because it needs multiline extended regular expressions, which bash does not have in that form), and putting that into nfs-utils will NOT cause a runtime dependency to /sbin/mount.nfs4 or somehow break DRACUT/initramfs. It's just a utility *script*. > > - This feature will not be provided for NFSv3 > > Why shouldn't mount.nfs also support using an NFS URL to mount an > NFSv3-only server? Isn't this simply a matter of letting mount.nfs > negotiate down to NFSv3 if needed? Two issues: 1. I consider NFSv2/NFSv3/NFSv4.0 as obsolete 2. It would be much more complex because we need to think about how to encode rpcbind&transport parameters, e.g. in cases of issues like custom rpcbind+mountd ports, and/or UDP. That will quickly escalate and require lots of debates. We had that debate already in ~~2006 when I was at SUN Microsystems, and there was never a consensus on how to do nfs://-URLs for NFSv3 outside WebNFS. I think it can be done, IMHO but this is way outside of the scope of this patch, which is mainly intended to fix some *URGENT* issues like paths with non-ASCII characters for the CJKV people and implement "hostport" notation (e.g. keep hostname+port in one string together). > General comments: > > The white space below looks mangled. That needs to be fixed before > this patch can be applied. Yes, I know, that is a problem that I only have the choice between GoogleMail or MS Outlook at work. That's why I provided kpaste.net links (https://nrubsig.kpaste.net/e8c5cb?raw and https://nrubsig.kpaste.net/e8c5cb). I'll try to set up git-imap-send in the future, but that needs time to convince our IT. This should work for the v2 patch: ---- snip ---- git clone git://git.linux-nfs.org/projects/steved/nfs-utils.git cd nfs-utils/ wget 'https://nrubsig.kpaste.net/e8c5cb?raw' dos2unix e8c5cb\@raw patch -p1 --dry-run <e8c5cb\@raw patch -p1 <e8c5cb\@raw ---- snip ---- > IMO, man page updates are needed along with this code change. Could we please move this to a separate patch set ? > IMO, using a URL parser library might be better for us in the > long run (eg, more secure) than adding our own little > implementation. FedFS used liburiparser. The liburiparser is a bit of an overkill, but I can look into it. I tried to keep it simple for ms-nfs41-client (see below), because otherwise I would've to deal with another DLL (which are far worse than any *.so issue I've seen on Solaris/Linux). The current urlparser1.[ch] was written for OpenSolaris long ago, and then updated for the Windows ms-nfs41-client project (see https://github.com/kofemann/ms-nfs41-client/tree/master/mount ; almost the same code, except there are additions for wide-char streams and MS Visual Studio) and is being maintained for that and several in-house projects, so maintenance is guaranteed (that's why my manager (Marvin Wenzel) signed it off, too). [snip] > > ---- snip ---- > > From e7b5a3ac981727e4585288bd66d1a9b2dea045dc Mon Sep 17 00:00:00 2001 > > From: Roland Mainz <roland.mainz@xxxxxxxxxxx> > > Date: Fri, 6 Dec 2024 10:58:48 +0100 > > Subject: [PATCH] mount.nfs4: Add support for nfs://-URLs > > > > Add support for RFC 2224-style nfs://-URLs as alternative to the > > traditional hostname:/path+-o port=<tcp-port> notation, > > providing standardised, extensible, single-string, crossplatform, > > portable, Character-Encoding independent (e.g. mount point with > > As above: s/mount point/export path OK Anything else ? ---- Bye, Roland -- __ . . __ (o.\ \/ /.o) roland.mainz@xxxxxxxxxxx \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 3992797 (;O/ \/ \O;)
