On Fri, Dec 6, 2024 at 4:54 PM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote: > > Hi Roland, thanks for posting. > > Here are some initial review comments to get the ball rolling. > > > On 12/6/24 5:54 AM, Roland Mainz wrote: > > Hi! > > > > ---- > > > > 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 > > (When/if you need to repost, you should move this introductory text into > a cover letter.) > > > > 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. > > > > * 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). > > > > - 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. > > I wonder how distributions will test the ability to mount > percent-escaped path names, though. Maybe not upstream's problem. > > > > - 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? > > > General comments: > > The white space below looks mangled. That needs to be fixed before > this patch can be applied. > > IMO, man page updates are needed along with this code change. > > 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. Yeah, another library dependency for Debian? First you try to invade Debian with libxml2 via backdoor, and now you try to add liburlparser? At that point I would suggest that Debian just forks nfs-utils and yanks the whole libxml&liburlparser garbage out and replace it with a simple line parser. Does the same job and doesn't litter Debian Mark -- IT Infrastructure Consultant Windows, Linux
