| Message ID | CAKAoaQmaf7d01vK4TfR+=8k4CVN-CX3ESPFh88EaxvcOAQDWtQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | mount.nfs: Add support for nfs://-URLs ... | expand |
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. Again, Steve, I would cut an nfs-utils release now, and then add NFS URL support just after releasing so it has a some time to soak before the next release. > ---- snip ---- > From e7b5a3ac981727e4585288bd66d1a9b2dea045dc Mon Sep 17 00:00:00 2001 > From: Roland Mainz <roland.mainz@nrubsig.org> > 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 > Japanese, Chinese, French etc. characters) and ASCII-compatible > descriptions of NFSv4 server resources (exports). > > Reviewed-by: Martin Wege <martin.l.wege@gmail.com> > Signed-off-by: Cedric Blancher <cedric.blancher@gmail.com> > --- > utils/mount/Makefile.am | 3 +- > utils/mount/nfs4mount.c | 69 +++++++- > utils/mount/nfsmount.c | 93 ++++++++-- > utils/mount/parse_dev.c | 67 ++++++-- > utils/mount/stropts.c | 96 ++++++++++- > utils/mount/urlparser1.c | 358 +++++++++++++++++++++++++++++++++++++++ > utils/mount/urlparser1.h | 60 +++++++ > utils/mount/utils.c | 155 +++++++++++++++++ > utils/mount/utils.h | 23 +++ > 9 files changed, 887 insertions(+), 37 deletions(-) > create mode 100644 utils/mount/urlparser1.c > create mode 100644 utils/mount/urlparser1.h > > diff --git a/utils/mount/Makefile.am b/utils/mount/Makefile.am > index 83a8ee1c..0e4cab3e 100644 > --- a/utils/mount/Makefile.am > +++ b/utils/mount/Makefile.am > @@ -13,7 +13,8 @@ sbin_PROGRAMS = mount.nfs > EXTRA_DIST = nfsmount.conf $(man8_MANS) $(man5_MANS) > mount_common = error.c network.c token.c \ > parse_opt.c parse_dev.c \ > - nfsmount.c nfs4mount.c stropts.c\ > + nfsmount.c nfs4mount.c \ > + urlparser1.c urlparser1.h stropts.c \ > mount_constants.h error.h network.h token.h \ > parse_opt.h parse_dev.h \ > nfs4_mount.h stropts.h version.h \ > diff --git a/utils/mount/nfs4mount.c b/utils/mount/nfs4mount.c > index 0fe142a7..8e4fbf30 100644 > --- a/utils/mount/nfs4mount.c > +++ b/utils/mount/nfs4mount.c > @@ -50,8 +50,10 @@ > #include "mount_constants.h" > #include "nfs4_mount.h" > #include "nfs_mount.h" > +#include "urlparser1.h" > #include "error.h" > #include "network.h" > +#include "utils.h" > > #if defined(VAR_LOCK_DIR) > #define DEFAULT_DIR VAR_LOCK_DIR > @@ -182,7 +184,7 @@ int nfs4mount(const char *spec, const char *node, int flags, > int num_flavour = 0; > int ip_addr_in_opts = 0; > > - char *hostname, *dirname, *old_opts; > + char *hostname, *dirname, *mb_dirname = NULL, *old_opts; > char new_opts[1024]; > char *opt, *opteq; > char *s; > @@ -192,15 +194,66 @@ int nfs4mount(const char *spec, const char > *node, int flags, > int retry; > int retval = EX_FAIL; > time_t timeout, t; > + int nfs_port = NFS_PORT; > + parsed_nfs_url pnu; > + > + (void)memset(&pnu, 0, sizeof(parsed_nfs_url)); > > if (strlen(spec) >= sizeof(hostdir)) { > nfs_error(_("%s: excessively long host:dir argument\n"), > progname); > goto fail; > } > - strcpy(hostdir, spec); > - if (parse_devname(hostdir, &hostname, &dirname)) > - goto fail; > + > + /* > + * Support nfs://-URLS per RFC 2224 ("NFS URL > + * SCHEME", see https://www.rfc-editor.org/rfc/rfc2224.html), > + * including custom port (nfs://hostname@port/path/...) > + * and URL parameter (e.g. nfs://.../?param1=val1¶m2=val2 > + * support > + */ > + if (is_spec_nfs_url(spec)) { > + if (!mount_parse_nfs_url(spec, &pnu)) { > + goto fail; > + } > + > + /* > + * |pnu.uctx->path| is in UTF-8, but we need the data > + * in the current local locale's encoding, as mount(2) > + * does not have something like a |MS_UTF8_SPEC| flag > + * to indicate that the input path is in UTF-8, > + * independently of the current locale > + */ > + hostname = pnu.uctx->hostport.hostname; > + dirname = mb_dirname = utf8str2mbstr(pnu.uctx->path); > + > + (void)snprintf(hostdir, sizeof(hostdir), "%s:/%s", > + hostname, dirname); > + spec = hostdir; > + > + if (pnu.uctx->hostport.port != -1) { > + nfs_port = pnu.uctx->hostport.port; > + } > + > + /* > + * Values added here based on URL parameters > + * should be added the front of the list of options, > + * so users can override the nfs://-URL given default. > + * > + * FIXME: We do not do that here for |MS_RDONLY|! > + */ > + if (pnu.mount_params.read_only != TRIS_BOOL_NOT_SET) { > + if (pnu.mount_params.read_only) > + flags |= MS_RDONLY; > + else > + flags &= ~MS_RDONLY; > + } > + } else { > + (void)strcpy(hostdir, spec); > + > + if (parse_devname(hostdir, &hostname, &dirname)) > + goto fail; > + } > > if (fill_ipv4_sockaddr(hostname, &server_addr)) > goto fail; > @@ -247,7 +300,7 @@ int nfs4mount(const char *spec, const char *node, int flags, > /* > * NFSv4 specifies that the default port should be 2049 > */ > - server_addr.sin_port = htons(NFS_PORT); > + server_addr.sin_port = htons(nfs_port); > > /* parse options */ > > @@ -474,8 +527,14 @@ int nfs4mount(const char *spec, const char *node, > int flags, > } > } > > + mount_free_parse_nfs_url(&pnu); > + free(mb_dirname); > + > return EX_SUCCESS; > > fail: > + mount_free_parse_nfs_url(&pnu); > + free(mb_dirname); > + > return retval; > } > diff --git a/utils/mount/nfsmount.c b/utils/mount/nfsmount.c > index a1c92fe8..e61d718a 100644 > --- a/utils/mount/nfsmount.c > +++ b/utils/mount/nfsmount.c > @@ -63,11 +63,13 @@ > #include "xcommon.h" > #include "mount.h" > #include "nfs_mount.h" > +#include "urlparser1.h" > #include "mount_constants.h" > #include "nls.h" > #include "error.h" > #include "network.h" > #include "version.h" > +#include "utils.h" > > #ifdef HAVE_RPCSVC_NFS_PROT_H > #include <rpcsvc/nfs_prot.h> > @@ -493,7 +495,7 @@ nfsmount(const char *spec, const char *node, int flags, > char **extra_opts, int fake, int running_bg) > { > char hostdir[1024]; > - char *hostname, *dirname, *old_opts, *mounthost = NULL; > + char *hostname, *dirname, *mb_dirname = NULL, *old_opts, *mounthost = NULL; > char new_opts[1024], cbuf[1024]; > static struct nfs_mount_data data; > int val; > @@ -521,29 +523,79 @@ nfsmount(const char *spec, const char *node, int flags, > time_t t; > time_t prevt; > time_t timeout; > + int nfsurl_port = -1; > + parsed_nfs_url pnu; > + > + (void)memset(&pnu, 0, sizeof(parsed_nfs_url)); > > if (strlen(spec) >= sizeof(hostdir)) { > nfs_error(_("%s: excessively long host:dir argument"), > progname); > goto fail; > } > - strcpy(hostdir, spec); > - if ((s = strchr(hostdir, ':'))) { > - hostname = hostdir; > - dirname = s + 1; > - *s = '\0'; > - /* Ignore all but first hostname in replicated mounts > - until they can be fully supported. (mack@sgi.com) */ > - if ((s = strchr(hostdir, ','))) { > + > + /* > + * Support nfs://-URLS per RFC 2224 ("NFS URL > + * SCHEME", see https://www.rfc-editor.org/rfc/rfc2224.html), > + * including custom port (nfs://hostname@port/path/...) > + * and URL parameter (e.g. nfs://.../?param1=val1¶m2=val2 > + * support > + */ > + if (is_spec_nfs_url(spec)) { > + if (!mount_parse_nfs_url(spec, &pnu)) { > + goto fail; > + } > + > + /* > + * |pnu.uctx->path| is in UTF-8, but we need the data > + * in the current local locale's encoding, as mount(2) > + * does not have something like a |MS_UTF8_SPEC| flag > + * to indicate that the input path is in UTF-8, > + * independently of the current locale > + */ > + hostname = pnu.uctx->hostport.hostname; > + dirname = mb_dirname = utf8str2mbstr(pnu.uctx->path); > + > + (void)snprintf(hostdir, sizeof(hostdir), "%s:/%s", > + hostname, dirname); > + spec = hostdir; > + > + if (pnu.uctx->hostport.port != -1) { > + nfsurl_port = pnu.uctx->hostport.port; > + } > + > + /* > + * Values added here based on URL parameters > + * should be added the front of the list of options, > + * so users can override the nfs://-URL given default. > + * > + * FIXME: We do not do that here for |MS_RDONLY|! > + */ > + if (pnu.mount_params.read_only != TRIS_BOOL_NOT_SET) { > + if (pnu.mount_params.read_only) > + flags |= MS_RDONLY; > + else > + flags &= ~MS_RDONLY; > + } > + } else { > + (void)strcpy(hostdir, spec); > + if ((s = strchr(hostdir, ':'))) { > + hostname = hostdir; > + dirname = s + 1; > *s = '\0'; > - nfs_error(_("%s: warning: " > - "multiple hostnames not supported"), > + /* Ignore all but first hostname in replicated mounts > + until they can be fully supported. (mack@sgi.com) */ > + if ((s = strchr(hostdir, ','))) { > + *s = '\0'; > + nfs_error(_("%s: warning: " > + "multiple hostnames not supported"), > progname); > - } > - } else { > - nfs_error(_("%s: directory to mount not in host:dir format"), > + } > + } else { > + nfs_error(_("%s: directory to mount not in host:dir format"), > progname); > - goto fail; > + goto fail; > + } > } > > if (!nfs_gethostbyname(hostname, nfs_saddr)) > @@ -579,6 +631,14 @@ nfsmount(const char *spec, const char *node, int flags, > memset(nfs_pmap, 0, sizeof(*nfs_pmap)); > nfs_pmap->pm_prog = NFS_PROGRAM; > > + if (nfsurl_port != -1) { > + /* > + * Set custom TCP port defined by a nfs://-URL here, > + * so $ mount -o port ... # can be used to override > + */ > + nfs_pmap->pm_port = nfsurl_port; > + } > + > /* parse options */ > new_opts[0] = 0; > if (!parse_options(old_opts, &data, &bg, &retry, &mnt_server, &nfs_server, > @@ -863,10 +923,13 @@ noauth_flavors: > } > } > > + mount_free_parse_nfs_url(&pnu); > + > return EX_SUCCESS; > > /* abort */ > fail: > + mount_free_parse_nfs_url(&pnu); > if (fsock != -1) > close(fsock); > return retval; > diff --git a/utils/mount/parse_dev.c b/utils/mount/parse_dev.c > index 2ade5d5d..d9f8cf59 100644 > --- a/utils/mount/parse_dev.c > +++ b/utils/mount/parse_dev.c > @@ -27,6 +27,8 @@ > #include "xcommon.h" > #include "nls.h" > #include "parse_dev.h" > +#include "urlparser1.h" > +#include "utils.h" > > #ifndef NFS_MAXHOSTNAME > #define NFS_MAXHOSTNAME (255) > @@ -179,17 +181,62 @@ static int nfs_parse_square_bracket(const char *dev, > } > > /* > - * RFC 2224 says an NFS client must grok "public file handles" to > - * support NFS URLs. Linux doesn't do that yet. Print a somewhat > - * helpful error message in this case instead of pressing forward > - * with the mount request and failing with a cryptic error message > - * later. > + * Support nfs://-URLS per RFC 2224 ("NFS URL > + * SCHEME", see https://www.rfc-editor.org/rfc/rfc2224.html), > + * including port support (nfs://hostname@port/path/...) > */ > -static int nfs_parse_nfs_url(__attribute__((unused)) const char *dev, > - __attribute__((unused)) char **hostname, > - __attribute__((unused)) char **pathname) > +static int nfs_parse_nfs_url(const char *dev, > + char **out_hostname, > + char **out_pathname) > { > - nfs_error(_("%s: NFS URLs are not supported"), progname); > + parsed_nfs_url pnu; > + > + if (out_hostname) > + *out_hostname = NULL; > + if (out_pathname) > + *out_pathname = NULL; > + > + /* > + * Support nfs://-URLS per RFC 2224 ("NFS URL > + * SCHEME", see https://www.rfc-editor.org/rfc/rfc2224.html), > + * including custom port (nfs://hostname@port/path/...) > + * and URL parameter (e.g. nfs://.../?param1=val1¶m2=val2 > + * support > + */ > + if (!mount_parse_nfs_url(dev, &pnu)) { > + goto fail; > + } > + > + if (pnu.uctx->hostport.port != -1) { > + /* NOP here, unless we switch from hostname to hostport */ > + } > + > + if (out_hostname) > + *out_hostname = strdup(pnu.uctx->hostport.hostname); > + if (out_pathname) > + *out_pathname = utf8str2mbstr(pnu.uctx->path); > + > + if (((out_hostname)?(*out_hostname == NULL):0) || > + ((out_pathname)?(*out_pathname == NULL):0)) { > + nfs_error(_("%s: out of memory"), > + progname); > + goto fail; > + } > + > + mount_free_parse_nfs_url(&pnu); > + > + return 1; > + > +fail: > + mount_free_parse_nfs_url(&pnu); > + if (out_hostname) { > + free(*out_hostname); > + *out_hostname = NULL; > + } > + if (out_pathname) { > + free(*out_pathname); > + *out_pathname = NULL; > + } > return 0; > } > > @@ -223,7 +270,7 @@ int nfs_parse_devname(const char *devname, > return nfs_pdn_nomem_err(); > if (*dev == '[') > result = nfs_parse_square_bracket(dev, hostname, pathname); > - else if (strncmp(dev, "nfs://", 6) == 0) > + else if (is_spec_nfs_url(dev)) > result = nfs_parse_nfs_url(dev, hostname, pathname); > else > result = nfs_parse_simple_hostname(dev, hostname, pathname); > diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c > index 23f0a8c0..ad92ab78 100644 > --- a/utils/mount/stropts.c > +++ b/utils/mount/stropts.c > @@ -42,6 +42,7 @@ > #include "nls.h" > #include "nfsrpc.h" > #include "mount_constants.h" > +#include "urlparser1.h" > #include "stropts.h" > #include "error.h" > #include "network.h" > @@ -50,6 +51,7 @@ > #include "parse_dev.h" > #include "conffile.h" > #include "misc.h" > +#include "utils.h" > > #ifndef NFS_PROGRAM > #define NFS_PROGRAM (100003) > @@ -643,24 +645,106 @@ static int nfs_sys_mount(struct nfsmount_info > *mi, struct mount_options *opts) > { > char *options = NULL; > int result; > + int nfs_port = 2049; > > if (mi->fake) > return 1; > > - if (po_join(opts, &options) == PO_FAILED) { > - errno = EIO; > - return 0; > - } > + /* > + * Support nfs://-URLS per RFC 2224 ("NFS URL > + * SCHEME", see https://www.rfc-editor.org/rfc/rfc2224.html), > + * including custom port (nfs://hostname@port/path/...) > + * and URL parameter (e.g. nfs://.../?param1=val1¶m2=val2 > + * support > + */ > + if (is_spec_nfs_url(mi->spec)) { > + parsed_nfs_url pnu; > + char *mb_path; > + char mount_source[1024]; > + > + if (!mount_parse_nfs_url(mi->spec, &pnu)) { > + result = 1; > + errno = EINVAL; > + goto done; > + } > + > + /* > + * |pnu.uctx->path| is in UTF-8, but we need the data > + * in the current local locale's encoding, as mount(2) > + * does not have something like a |MS_UTF8_SPEC| flag > + * to indicate that the input path is in UTF-8, > + * independently of the current locale > + */ > + mb_path = utf8str2mbstr(pnu.uctx->path); > + if (!mb_path) { > + nfs_error(_("%s: Could not convert path to local encoding."), > + progname); > + mount_free_parse_nfs_url(&pnu); > + result = 1; > + errno = EINVAL; > + goto done; > + } > + > + (void)snprintf(mount_source, sizeof(mount_source), > + "%s:/%s", > + pnu.uctx->hostport.hostname, > + mb_path); > + free(mb_path); > + > + if (pnu.uctx->hostport.port != -1) { > + nfs_port = pnu.uctx->hostport.port; > + } > > - result = mount(mi->spec, mi->node, mi->type, > + /* > + * Insert "port=" option with the value from the nfs:// > + * URL at the beginning of the list of options, so > + * users can override it with $ mount.nfs4 -o port= #, > + * e.g. > + * $ mount.nfs4 -o port=1234 nfs://10.49.202.230:400//bigdisk /mnt4 # > + * should use port 1234, and not port 400 as specified > + * in the URL. > + */ > + char portoptbuf[5+32+1]; > + (void)snprintf(portoptbuf, sizeof(portoptbuf), "port=%d", nfs_port); > + (void)po_insert(opts, portoptbuf); > + > + if (pnu.mount_params.read_only != TRIS_BOOL_NOT_SET) { > + if (pnu.mount_params.read_only) > + mi->flags |= MS_RDONLY; > + else > + mi->flags &= ~MS_RDONLY; > + } > + > + mount_free_parse_nfs_url(&pnu); > + > + if (po_join(opts, &options) == PO_FAILED) { > + errno = EIO; > + result = 1; > + goto done; > + } > + > + result = mount(mount_source, mi->node, mi->type, > + mi->flags & ~(MS_USER|MS_USERS), options); > + free(options); > + } else { > + if (po_join(opts, &options) == PO_FAILED) { > + errno = EIO; > + result = 1; > + goto done; > + } > + > + result = mount(mi->spec, mi->node, mi->type, > mi->flags & ~(MS_USER|MS_USERS), options); > - free(options); > + free(options); > + } > > if (verbose && result) { > int save = errno; > nfs_error(_("%s: mount(2): %s"), progname, strerror(save)); > errno = save; > } > + > +done: > return !result; > } > > diff --git a/utils/mount/urlparser1.c b/utils/mount/urlparser1.c > new file mode 100644 > index 00000000..d4c6f339 > --- /dev/null > +++ b/utils/mount/urlparser1.c > @@ -0,0 +1,358 @@ > +/* > + * MIT License > + * > + * Copyright (c) 2024 Roland Mainz <roland.mainz@nrubsig.org> > + * > + * Permission is hereby granted, free of charge, to any person obtaining a copy > + * of this software and associated documentation files (the > "Software"), to deal > + * in the Software without restriction, including without limitation the rights > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell > + * copies of the Software, and to permit persons to whom the Software is > + * furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be > included in all > + * copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE > + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > ARISING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > DEALINGS IN THE > + * SOFTWARE. > + */ > + > +/* urlparser1.c - simple URL parser */ > + > +#include <stdlib.h> > +#include <stdbool.h> > +#include <string.h> > +#include <ctype.h> > +#include <stdio.h> > + > +#ifdef DBG_USE_WIDECHAR > +#include <wchar.h> > +#include <locale.h> > +#include <io.h> > +#include <fcntl.h> > +#endif /* DBG_USE_WIDECHAR */ > + > +#include "urlparser1.h" > + > +typedef struct _url_parser_context_private { > + url_parser_context c; > + > + /* Private data */ > + char *parameter_string_buff; > +} url_parser_context_private; > + > +#define MAX_URL_PARAMETERS 256 > + > +/* > + * Original extended regular expression: > + * > + * "^" > + * "(.+?)" // scheme > + * "://" // '://' > + * "(" // login > + * "(?:" > + * "(.+?)" // user (optional) > + * "(?::(.+))?" // password (optional) > + * "@" > + * ")?" > + * "(" // hostport > + * "(.+?)" // host > + * "(?::([[:digit:]]+))?" // port (optional) > + * ")" > + * ")" > + * "(?:/(.*?))?" // path (optional) > + * "(?:\?(.*?))?" // URL parameters (optional) > + * "$" > + */ > + > +#define DBGNULLSTR(s) (((s)!=NULL)?(s):"<NULL>") > +#if 0 || defined(TEST_URLPARSER) > +#define D(x) x > +#else > +#define D(x) > +#endif > + > +#ifdef DBG_USE_WIDECHAR > +/* > + * Use wide-char APIs on WIN32, otherwise we cannot output > + * Japanese/Chinese/etc correctly > + */ > +#define DBG_PUTS(str, fp) fputws(L"" str, (fp)) > +#define DBG_PUTC(c, fp) fputwc(btowc(c), (fp)) > +#define DBG_PRINTF(fp, fmt, ...) fwprintf((fp), L"" fmt, __VA_ARGS__) > +#else > +#define DBG_PUTS(str, fp) fputs((str), (fp)) > +#define DBG_PUTC(c, fp) fputc((c), (fp)) > +#define DBG_PRINTF(fp, fmt, ...) fprintf((fp), fmt, __VA_ARGS__) > +#endif /* DBG_USE_WIDECHAR */ > + > +static > +void urldecodestr(char *outbuff, const char *buffer, size_t len) > +{ > + size_t i, j; > + > + for (i = j = 0 ; i < len ; ) { > + switch (buffer[i]) { > + case '%': > + if ((i + 2) < len) { > + if (isxdigit((int)buffer[i+1]) && isxdigit((int)buffer[i+2])) { > + const char hexstr[3] = { > + buffer[i+1], > + buffer[i+2], > + '\0' > + }; > + outbuff[j++] = (unsigned char)strtol(hexstr, NULL, 16); > + i += 3; > + } else { > + /* invalid hex digit */ > + outbuff[j++] = buffer[i]; > + i++; > + } > + } else { > + /* incomplete hex digit */ > + outbuff[j++] = buffer[i]; > + i++; > + } > + break; > + case '+': > + outbuff[j++] = ' '; > + i++; > + break; > + default: > + outbuff[j++] = buffer[i++]; > + break; > + } > + } > + > + outbuff[j] = '\0'; > +} > + > +url_parser_context *url_parser_create_context(const char *in_url, > unsigned int flags) > +{ > + url_parser_context_private *uctx; > + char *s; > + size_t in_url_len; > + size_t context_len; > + > + /* |flags| is for future extensions */ > + (void)flags; > + > + if (!in_url) > + return NULL; > + > + in_url_len = strlen(in_url); > + > + context_len = sizeof(url_parser_context_private) + > + ((in_url_len+1)*6) + > + (sizeof(url_parser_name_value)*MAX_URL_PARAMETERS)+sizeof(void*); > + uctx = malloc(context_len); > + if (!uctx) > + return NULL; > + > + s = (void *)(uctx+1); > + uctx->c.in_url = s; s+= in_url_len+1; > + (void)strcpy(uctx->c.in_url, in_url); > + uctx->c.scheme = s; s+= in_url_len+1; > + uctx->c.login.username = s; s+= in_url_len+1; > + uctx->c.hostport.hostname = s; s+= in_url_len+1; > + uctx->c.path = s; s+= in_url_len+1; > + uctx->c.hostport.port = -1; > + uctx->c.num_parameters = -1; > + uctx->c.parameters = (void *)s; s+= > (sizeof(url_parser_name_value)*MAX_URL_PARAMETERS)+sizeof(void*); > + uctx->parameter_string_buff = s; s+= in_url_len+1; > + > + return &uctx->c; > +} > + > +int url_parser_parse(url_parser_context *ctx) > +{ > + url_parser_context_private *uctx = (url_parser_context_private *)ctx; > + > + D((void)DBG_PRINTF(stderr, "## parser in_url='%s'\n", uctx->c.in_url)); > + > + char *s; > + const char *urlstr = uctx->c.in_url; > + size_t slen; > + > + s = strstr(urlstr, "://"); > + if (!s) { > + D((void)DBG_PUTS("url_parser: Not an URL\n", stderr)); > + return -1; > + } > + > + slen = s-urlstr; > + (void)memcpy(uctx->c.scheme, urlstr, slen); > + uctx->c.scheme[slen] = '\0'; > + urlstr += slen + 3; > + > + D((void)DBG_PRINTF(stdout, "scheme='%s', rest='%s'\n", > uctx->c.scheme, urlstr)); > + > + s = strstr(urlstr, "@"); > + if (s) { > + /* URL has user/password */ > + slen = s-urlstr; > + urldecodestr(uctx->c.login.username, urlstr, slen); > + urlstr += slen + 1; > + > + s = strstr(uctx->c.login.username, ":"); > + if (s) { > + /* found passwd */ > + uctx->c.login.passwd = s+1; > + *s = '\0'; > + } > + else { > + uctx->c.login.passwd = NULL; > + } > + > + /* catch password-only URLs */ > + if (uctx->c.login.username[0] == '\0') > + uctx->c.login.username = NULL; > + } > + else { > + uctx->c.login.username = NULL; > + uctx->c.login.passwd = NULL; > + } > + > + D((void)DBG_PRINTF(stdout, "login='%s', passwd='%s', rest='%s'\n", > + DBGNULLSTR(uctx->c.login.username), > + DBGNULLSTR(uctx->c.login.passwd), > + DBGNULLSTR(urlstr))); > + > + char *raw_parameters; > + > + uctx->c.num_parameters = 0; > + raw_parameters = strstr(urlstr, "?"); > + /* Do we have a non-empty parameter string ? */ > + if (raw_parameters && (raw_parameters[1] != '\0')) { > + *raw_parameters++ = '\0'; > + D((void)DBG_PRINTF(stdout, "raw parameters = '%s'\n", raw_parameters)); > + > + char *ps = raw_parameters; > + char *pv; /* parameter value */ > + char *na; /* next '&' */ > + char *pb = uctx->parameter_string_buff; > + char *pname; > + char *pvalue; > + ssize_t pi; > + > + for (pi = 0; pi < MAX_URL_PARAMETERS ; pi++) { > + pname = ps; > + > + /* > + * Handle parameters without value, > + * e.g. "path?name1&name2=value2" > + */ > + na = strstr(ps, "&"); > + pv = strstr(ps, "="); > + if (pv && (na?(na > pv):true)) { > + *pv++ = '\0'; > + pvalue = pv; > + ps = pv; > + } > + else { > + pvalue = NULL; > + } > + > + if (na) { > + *na++ = '\0'; > + } > + > + /* URLDecode parameter name */ > + urldecodestr(pb, pname, strlen(pname)); > + uctx->c.parameters[pi].name = pb; > + pb += strlen(uctx->c.parameters[pi].name)+1; > + > + /* URLDecode parameter value */ > + if (pvalue) { > + urldecodestr(pb, pvalue, strlen(pvalue)); > + uctx->c.parameters[pi].value = pb; > + pb += strlen(uctx->c.parameters[pi].value)+1; > + } > + else { > + uctx->c.parameters[pi].value = NULL; > + } > + > + /* Next '&' ? */ > + if (!na) > + break; > + > + ps = na; > + } > + > + uctx->c.num_parameters = pi+1; > + } > + > + s = strstr(urlstr, "/"); > + if (s) { > + /* URL has hostport */ > + slen = s-urlstr; > + urldecodestr(uctx->c.hostport.hostname, urlstr, slen); > + urlstr += slen + 1; > + > + /* > + * check for addresses within '[' and ']', like > + * IPv6 addresses > + */ > + s = uctx->c.hostport.hostname; > + if (s[0] == '[') > + s = strstr(s, "]"); > + > + if (s == NULL) { > + D((void)DBG_PUTS("url_parser: Unmatched '[' in hostname\n", stderr)); > + return -1; > + } > + > + s = strstr(s, ":"); > + if (s) { > + /* found port number */ > + uctx->c.hostport.port = atoi(s+1); > + *s = '\0'; > + } > + } > + else { > + (void)strcpy(uctx->c.hostport.hostname, urlstr); > + uctx->c.path = NULL; > + urlstr = NULL; > + } > + > + D( > + (void)DBG_PRINTF(stdout, > + "hostport='%s', port=%d, rest='%s', num_parameters=%d\n", > + DBGNULLSTR(uctx->c.hostport.hostname), > + uctx->c.hostport.port, > + DBGNULLSTR(urlstr), > + (int)uctx->c.num_parameters); > + ); > + > + > + D( > + ssize_t dpi; > + for (dpi = 0 ; dpi < uctx->c.num_parameters ; dpi++) { > + (void)DBG_PRINTF(stdout, > + "param[%d]: name='%s'/value='%s'\n", > + (int)dpi, > + uctx->c.parameters[dpi].name, > + DBGNULLSTR(uctx->c.parameters[dpi].value)); > + } > + ); > + > + if (!urlstr) { > + goto done; > + } > + > + urldecodestr(uctx->c.path, urlstr, strlen(urlstr)); > + D((void)DBG_PRINTF(stdout, "path='%s'\n", uctx->c.path)); > + > +done: > + return 0; > +} > + > +void url_parser_free_context(url_parser_context *c) > +{ > + free(c); > +} > diff --git a/utils/mount/urlparser1.h b/utils/mount/urlparser1.h > new file mode 100644 > index 00000000..515eea9d > --- /dev/null > +++ b/utils/mount/urlparser1.h > @@ -0,0 +1,60 @@ > +/* > + * MIT License > + * > + * Copyright (c) 2024 Roland Mainz <roland.mainz@nrubsig.org> > + * > + * Permission is hereby granted, free of charge, to any person obtaining a copy > + * of this software and associated documentation files (the > "Software"), to deal > + * in the Software without restriction, including without limitation the rights > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell > + * copies of the Software, and to permit persons to whom the Software is > + * furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be > included in all > + * copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE > + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > ARISING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > DEALINGS IN THE > + * SOFTWARE. > + */ > + > +/* urlparser1.h - header for simple URL parser */ > + > +#ifndef __URLPARSER1_H__ > +#define __URLPARSER1_H__ 1 > + > +#include <stdlib.h> > + > +typedef struct _url_parser_name_value { > + char *name; > + char *value; > +} url_parser_name_value; > + > +typedef struct _url_parser_context { > + char *in_url; > + > + char *scheme; > + struct { > + char *username; > + char *passwd; > + } login; > + struct { > + char *hostname; > + signed int port; > + } hostport; > + char *path; > + > + ssize_t num_parameters; > + url_parser_name_value *parameters; > +} url_parser_context; > + > +/* Prototypes */ > +url_parser_context *url_parser_create_context(const char *in_url, > unsigned int flags); > +int url_parser_parse(url_parser_context *uctx); > +void url_parser_free_context(url_parser_context *c); > + > +#endif /* !__URLPARSER1_H__ */ > diff --git a/utils/mount/utils.c b/utils/mount/utils.c > index b7562a47..2d4cfa5a 100644 > --- a/utils/mount/utils.c > +++ b/utils/mount/utils.c > @@ -28,6 +28,7 @@ > #include <unistd.h> > #include <sys/types.h> > #include <sys/stat.h> > +#include <iconv.h> > > #include "sockaddr.h" > #include "nfs_mount.h" > @@ -173,3 +174,157 @@ int nfs_umount23(const char *devname, char *string) > free(dirname); > return result; > } > + > +/* Convert UTF-8 string to multibyte string in the current locale */ > +char *utf8str2mbstr(const char *utf8_str) > +{ > + iconv_t cd; > + > + cd = iconv_open("UTF-8", ""); > + if (cd == (iconv_t)-1) { > + perror("utf8str2mbstr: iconv_open failed"); > + return NULL; > + } > + > + size_t inbytesleft = strlen(utf8_str); > + char *inbuf = (char *)utf8_str; > + size_t outbytesleft = inbytesleft*4+1; > + char *outbuf = malloc(outbytesleft); > + char *outbuf_orig = outbuf; > + > + if (!outbuf) { > + perror("utf8str2mbstr: out of memory"); > + (void)iconv_close(cd); > + return NULL; > + } > + > + int ret = iconv(cd, &inbuf, &inbytesleft, &outbuf, &outbytesleft); > + if (ret == -1) { > + perror("utf8str2mbstr: iconv() failed"); > + free(outbuf_orig); > + (void)iconv_close(cd); > + return NULL; > + } > + > + *outbuf = '\0'; > + > + (void)iconv_close(cd); > + return outbuf_orig; > +} > + > +/* fixme: We should use |bool|! */ > +int is_spec_nfs_url(const char *spec) > +{ > + return (!strncmp(spec, "nfs://", 6)); > +} > + > +int mount_parse_nfs_url(const char *spec, parsed_nfs_url *pnu) > +{ > + int result = 1; > + url_parser_context *uctx = NULL; > + > + (void)memset(pnu, 0, sizeof(parsed_nfs_url)); > + pnu->mount_params.read_only = TRIS_BOOL_NOT_SET; > + > + uctx = url_parser_create_context(spec, 0); > + if (!uctx) { > + nfs_error(_("%s: out of memory"), > + progname); > + result = 1; > + goto done; > + } > + > + if (url_parser_parse(uctx) < 0) { > + nfs_error(_("%s: Error parsing nfs://-URL."), > + progname); > + result = 1; > + goto done; > + } > + if (uctx->login.username || uctx->login.passwd) { > + nfs_error(_("%s: Username/Password are not defined for nfs://-URL."), > + progname); > + result = 1; > + goto done; > + } > + if (!uctx->path) { > + nfs_error(_("%s: Path missing in nfs://-URL."), > + progname); > + result = 1; > + goto done; > + } > + if (uctx->path[0] != '/') { > + nfs_error(_("%s: Relative nfs://-URLs are not supported."), > + progname); > + result = 1; > + goto done; > + } > + > + if (uctx->num_parameters > 0) { > + int pi; > + const char *pname; > + const char *pvalue; > + > + /* > + * Values added here based on URL parameters > + * should be added the front of the list of options, > + * so users can override the nfs://-URL given default. > + */ > + for (pi = 0; pi < uctx->num_parameters ; pi++) { > + pname = uctx->parameters[pi].name; > + pvalue = uctx->parameters[pi].value; > + > + if (!strcmp(pname, "rw")) { > + if ((pvalue == NULL) || (!strcmp(pvalue, "1"))) { > + pnu->mount_params.read_only = TRIS_BOOL_FALSE; > + } > + else if (!strcmp(pvalue, "0")) { > + pnu->mount_params.read_only = TRIS_BOOL_TRUE; > + } > + else { > + nfs_error(_("%s: Unsupported nfs://-URL " > + "parameter '%s' value '%s'."), > + progname, pname, pvalue); > + result = 1; > + goto done; > + } > + } > + else if (!strcmp(pname, "ro")) { > + if ((pvalue == NULL) || (!strcmp(pvalue, "1"))) { > + pnu->mount_params.read_only = TRIS_BOOL_TRUE; > + } > + else if (!strcmp(pvalue, "0")) { > + pnu->mount_params.read_only = TRIS_BOOL_FALSE; > + } > + else { > + nfs_error(_("%s: Unsupported nfs://-URL " > + "parameter '%s' value '%s'."), > + progname, pname, pvalue); > + result = 1; > + goto done; > + } > + } > + else { > + nfs_error(_("%s: Unsupported nfs://-URL " > + "parameter '%s'."), > + progname, pname); > + result = 1; > + goto done; > + } > + } > + } > + > + result = 0; > +done: > + if (result != 0) { > + url_parser_free_context(uctx); > + return 0; > + } > + > + pnu->uctx = uctx; > + return 1; > +} > + > +void mount_free_parse_nfs_url(parsed_nfs_url *pnu) > +{ > + url_parser_free_context(pnu->uctx); > +} > diff --git a/utils/mount/utils.h b/utils/mount/utils.h > index 224918ae..465c0a5e 100644 > --- a/utils/mount/utils.h > +++ b/utils/mount/utils.h > @@ -24,13 +24,36 @@ > #define _NFS_UTILS_MOUNT_UTILS_H > > #include "parse_opt.h" > +#include "urlparser1.h" > > +/* Boolean with three states: { not_set, false, true */ > +typedef signed char tristate_bool; > +#define TRIS_BOOL_NOT_SET (-1) > +#define TRIS_BOOL_TRUE (1) > +#define TRIS_BOOL_FALSE (0) > + > +#define TRIS_BOOL_GET_VAL(tsb, tsb_default) \ > + (((tsb)!=TRIS_BOOL_NOT_SET)?(tsb):(tsb_default)) > + > +typedef struct _parsed_nfs_url { > + url_parser_context *uctx; > + struct { > + tristate_bool read_only; > + } mount_params; > +} parsed_nfs_url; > + > +/* Prototypes */ > int discover_nfs_mount_data_version(int *string_ver); > void print_one(char *spec, char *node, char *type, char *opts); > void mount_usage(void); > void umount_usage(void); > int chk_mountpoint(const char *mount_point); > +char *utf8str2mbstr(const char *utf8_str); > +int is_spec_nfs_url(const char *spec); > > int nfs_umount23(const char *devname, char *string); > > +int mount_parse_nfs_url(const char *spec, parsed_nfs_url *pnu); > +void mount_free_parse_nfs_url(parsed_nfs_url *pnu); > + > #endif /* !_NFS_UTILS_MOUNT_UTILS_H */
On Fri, Dec 6, 2024 at 4:54 PM Chuck Lever <chuck.lever@oracle.com> 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
On 12/6/24 11:15 AM, Mark Liam Brown wrote: > On Fri, Dec 6, 2024 at 4:54 PM Chuck Lever <chuck.lever@oracle.com> wrote: >> 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 This is a political screed rather than a technical concern. For one thing, a fork certainly isn't needed to remove libxml2 or any other library dependency -- all distributors carry local patches to such packages. In any event, I had thought that the Linux community preferred to de-duplicate common functionality like this. If distributions want a less secure, harder-to-maintain solution, then they are welcome to speak up and explain themselves. That's what the review process is for.
On Fri, Dec 6, 2024 at 5:58 PM Chuck Lever <chuck.lever@oracle.com> wrote: > > On 12/6/24 11:15 AM, Mark Liam Brown wrote: > > On Fri, Dec 6, 2024 at 4:54 PM Chuck Lever <chuck.lever@oracle.com> wrote: > >> 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 > > This is a political screed rather than a technical concern. Nope. Stuffing the libxml2 garbage as dependiy to nfs-utils broke dracut in Debian and thus nfsroot support, and that is a REAL WORLD technical concern. > For one > thing, a fork certainly isn't needed to remove libxml2 or any other > library dependency -- all distributors carry local patches to such > packages. It is a concern that nfs-utils keep adding library dependencies for which there is no technical need. Junction support in nfs-utils could've used a simple tag=value format, instead a slow, resource-hungry and upgrade-antagonistic libxml2 was chosen. Real world users could not boot after that. So a fork of nfs-utils would jank out libxml2 and use a plain tag=value format for NFS junctions. So nay nay nay to more libraries, or keep it the liburiparser dep off by default. People who really want it can do an --enable-nfsurlsupportwithliburiparsergarbage. No one really needs exports and filenames with non-American characters anyway, the workaround is just mkdir /myjapanesefiles/, export that dir, and keep Japanese file names within that subtree. That would benefit people more because liburiparser.so.1630919828 would not be pulled in. Mark
On 12/6/24 12:13 PM, Mark Liam Brown wrote: > On Fri, Dec 6, 2024 at 5:58 PM Chuck Lever <chuck.lever@oracle.com> wrote: >> >> On 12/6/24 11:15 AM, Mark Liam Brown wrote: >>> On Fri, Dec 6, 2024 at 4:54 PM Chuck Lever <chuck.lever@oracle.com> wrote: >>>> 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 >> >> This is a political screed rather than a technical concern. > > Nope. Stuffing the libxml2 garbage as dependiy to nfs-utils broke > dracut in Debian and thus nfsroot support, and that is a REAL WORLD > technical concern. You should re-read your initial email. None of this detail is mentioned there, so it's not unexpected that a recipient might dismiss your concerns. Junction support in nfs-utils has always been controlled by the configure option "--enable-junctions" so that distributions who cannot or do not want to package libxml2 can build without that dependency. There are one or two bugs in this area which prevented this from working as intended. The intention has always been that distros and other packagers can choose not to pull in libxml2. >> For one >> thing, a fork certainly isn't needed to remove libxml2 or any other >> library dependency -- all distributors carry local patches to such >> packages. > > It is a concern that nfs-utils keep adding library dependencies for > which there is no technical need. Junction support in nfs-utils> could've used a simple tag=value format, instead a slow, > resource-hungry and upgrade-antagonistic libxml2 was chosen. > Real world users could not boot after that. This sounds like a packaging issue; that is, it's specific to the Debian distribution, not to whether nfs-utils has a new dependency. It was not a problem for SuSE or Redhat when they enabled junction support (RH enabled it years and years ago). Please post a link to bug reports that provide a clear description of the problem (and preferably also a root cause). This is honestly the first I've heard of such a problem; and if it truly is an upstream issue, it should be reported and dealt with here. > So a fork of nfs-utils would jank out libxml2 and use a plain > tag=value format for NFS junctions. It wouldn't be difficult to write a patch series to implement a KV-based junction format and teach mountd to look for that and the XML-based one for some transition period. Would you care to give that a try? > So nay nay nay to more libraries, or keep it the liburiparser dep off > by default. People who really want it can do an > --enable-nfsurlsupportwithliburiparsergarbage. Why didn't you simply suggest that before? In fact that is usually the way new features like this one are introduced to nfs-utils, so it would be a no-brainer if in fact the maintainer decides to replace the proposed ad hoc URL parser with a library. > No one really needs exports and filenames with non-American characters > anyway, the workaround is just mkdir /myjapanesefiles/, export that > dir, and keep Japanese file names within that subtree. You personally might not need to handle non-ASCII characters in your export paths. However, I'm told that most of the non-Eurocentric world wants to have support for mounting export paths with non-ASCII characters directly. So, although the workaround you describe works today, it's not something that any non-European administrator wants to use daily. I have no objection to handling NFS URIs in mount.nfs if no other code changes are needed. Most other NFS implementations have been able to deal with them properly for many years.
On Fri, Dec 6, 2024 at 4:54 PM Chuck Lever <chuck.lever@oracle.com> 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@nrubsig.org> > > 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
On 12/6/24 6:33 PM, Roland Mainz wrote: > On Fri, Dec 6, 2024 at 4:54 PM Chuck Lever <chuck.lever@oracle.com> 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 ? Have a look at git-send-email(1), and search for "cover". A cover letter is an introductory email, followed by the patches themselves in separate email items (usually organized as replies to the cover letter, but that depends on the email sending tool). >>> 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. NFS URLs are specified by RFC 2224, but extended by RFC 7532 Section 2.8.1: https://www.rfc-editor.org/rfc/rfc7532.html#section-2.8.1 Note this language: An NFS URI MUST contain both an authority and a path component. It MUST NOT contain a query component or a fragment component. Use of the familiar "nfs" scheme name is retained. IMHO this part of your implementation needs to be postponed until the standards are worked out. > 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. Fatal? What does "break stuff" mean, precisely? > 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 ? That would be good. >>> * 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. Some reviewers like to paraphrase to show their understanding -- basically if what I've repeated back to you doesn't reflect your understanding, here your opportunity to say "No, it works this way instead". > And the code will reject any relative URLs ($ grep -r > -F 'Relative nfs://-URLs are not supported.' #) ... That is right and proper ;-) >> 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*. That's fine. Testing components like this are generally packaged so that they are not installed in resource-constrained environments such as initramfs. >>> - 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 NFSv2 is obsolete, yes. The NFSv3 standard is not being updated, but NFSv3 is broadly deployed and implementations are actively maintained. It's not obsolete. NFSv4.0 is on its way out, but is not obsolete yet. I don't think it would be challenging to include support here. > 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. This is much more than is needed, IMO. NFSv3 can stick with the standard rpcbind port, an rpcbind query for MNT. My reading of the RFCs suggests that for NFSv3, the transport protocol is selected based on what both sides support. I think we want to avoid trying to build every bell and whistle into the mount.nfs NFS URL implementation. Just cover the basics, at least in the initial implementation. > 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 Er. Well maybe. It's up to the nfs-utils maintainer whether he wants to take a patch like that. However, our email-based review workflow means that the actual patch that every one has reviewed and signed off on is preserved in a public mail archive (along with the Acks, Tested-bys and Reviewed-bys). We don't have that when someone posts a link to a temporary paste bin. Paste bin submissions are not scalable for the maintainer because our tool chain is designed around taking an email submission and applying it directly. Pulling from a paste bin is a lot of manual work. We all have the same struggles with our corporate IT departments. > ---- snip ---- > >> IMO, man page updates are needed along with this code change. > > Could we please move this to a separate patch set ? That's up to the maintainer, but he usually requires adequate documentation along with new features. It's too easy to submit code changes and then put off the documentation for ever. >> 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. Here's the point of using a library: A popular library implementation has been very well reviewed and is used by a number of applications. That gives a high degree of confidence that there are fewer bugs (in particular, security-related bugs). The library might be actively maintained, and so we don't have to do that work. Something like liburiparser might be more than we need for this particular job, but code re-use is pretty foundational. If liburiparser isn't a good fit, look for something that is a better fit. > 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). OK, but you're /copying/ this into nfs-utils. How will our copy get updated over time? With an external library, bug fixes go to the library and nfs-utils gets them automatically when it is re-linked. > [snip] >>> ---- snip ---- >>> From e7b5a3ac981727e4585288bd66d1a9b2dea045dc Mon Sep 17 00:00:00 2001 >>> From: Roland Mainz <roland.mainz@nrubsig.org> >>> 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 ? You should expect review comments from others as well.
On Fri, Dec 6, 2024 at 12:10 PM Roland Mainz <roland.mainz@nrubsig.org> 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, > 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 '?'). Why wasn't this fixed earlier? > - URL parameters are supported, providing support for future extensions > > * Notes: > - Similar support for nfs://-URLs exists in other NFSv4.* > implementations, including Illumos, Windows ms-nfs41-client, > sahlberg/libnfs, ... OpenText NFS Solo also uses nfs URL > - 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 feature will not be provided for NFSv3 > > ---- snip ---- > From e7b5a3ac981727e4585288bd66d1a9b2dea045dc Mon Sep 17 00:00:00 2001 > From: Roland Mainz <roland.mainz@nrubsig.org> > 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 > Japanese, Chinese, French etc. characters) and ASCII-compatible > descriptions of NFSv4 server resources (exports). > > Reviewed-by: Martin Wege <martin.l.wege@gmail.com> > Signed-off-by: Cedric Blancher <cedric.blancher@gmail.com> > --- Sweet. LGTM
On 12/7/24 10:20 AM, Chuck Lever wrote: > On 12/6/24 6:33 PM, Roland Mainz wrote: >> On Fri, Dec 6, 2024 at 4:54 PM Chuck Lever <chuck.lever@oracle.com> >> wrote: >>>> - 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 > > NFSv2 is obsolete, yes. > > The NFSv3 standard is not being updated, but NFSv3 is broadly > deployed and implementations are actively maintained. It's not > obsolete. > > NFSv4.0 is on its way out, but is not obsolete yet. I don't > think it would be challenging to include support here. (Indeed, I presumed that /excluding/ minor version 0 would require some extra work. But maybe that's not true). >> 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. > > This is much more than is needed, IMO. NFSv3 can stick with the > standard rpcbind port, an rpcbind query for MNT. My reading of > the RFCs suggests that for NFSv3, the transport protocol is > selected based on what both sides support. > > I think we want to avoid trying to build every bell and whistle > into the mount.nfs NFS URL implementation. Just cover the basics, > at least in the initial implementation. > > >> 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). I don't understand your comment about scope. The scope of this patch, I thought, was to implement RFC 2224-compliant NFS URLs for the mount.nfs command line. NFSv3 is specified right there in that RFC. It seems to me that NFSv3 is indeed within scope, and NFSv4 is in fact not (if you want to be pedantic about it). And I don't see why handling NFSv3 should be difficult: The URL parser should simply translate the URL into equivalent command line options and pass those to the stropt code. That will handle NFS version and transport negotiation automatically, yes? Administrators will expect to be able to control the version negotiation behavior via settings in /etc/nfs.conf. If NFS URLs bypass mount.nfs's version negotiation (and therefore do not comply with the settings in /etc/nfs.conf), I think that is not correct and will be disappointing to administrators.
On Sat, 07 Dec 2024, Chuck Lever 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). Actually it isn't. You have already discussed the pub/root filehandle difference. The RFC doesn't know about v4. The RFC explicitly isn't a standard. So I wonder if this is the right approach to solve the need. What is the needed? Part of it seems to be non-ascii host names. Shouldn't we fix that for the existing syntax? What are the barriers? Part seems to be the inclusion of the port number. Is a "URL" really the best way to solve that need? Mount.nfs currently expects ":/" to separate host name from path. I think host:port:/path would be unambiguous providing "port" did not start "/". Do we really need the % encoding that the URL syntax gives us? If so - why? NeilBrown
On 12/7/24 3:53 PM, NeilBrown wrote: > On Sat, 07 Dec 2024, Chuck Lever 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). > > Actually it isn't. You have already discussed the pub/root filehandle > difference. RFC 2224 specifies both. Pub vs. root filehandles are discussed there, along with how standard NFS URLs describe either situation. > The RFC doesn't know about v4. The RFC explicitly isn't a > standard. RFC 7532 contains the NFSv4 bits. RFC 2224 is not a Normative standard, like all early NFS-related RFCs, but it is a specification that other implementations cleave to. RFC 7532 /is/ a Normative standard. > So I wonder if this is the right approach to solve the need. > > What is the needed? > Part of it seems to be non-ascii host names. Shouldn't we fix that for > the existing syntax? What are the barriers? Both non-ASCII hostnames (iDNA) and export paths can contain components with non-ASCII characters. The issue is how to specify certain code points when the client's locale might not support them. Using a URL seems to be the mechanism chosen by several other NFS implementations to deal with this problem. > Part seems to be the inclusion of the port number. Is a "URL" really > the best way to solve that need? > Mount.nfs currently expects ":/" to separate host name from path. > I think host:port:/path would be unambiguous providing "port" did not > start "/". There's also IPv6 presentation addresses, which contain ":" already. However host:port:/path is not something that other NFS implementations (eg that might share an automounter map) would understand. There is a significantly higher likelihood that those implementations would be able to interpret an NFS URI correctly. I'm not especially convinced by the arguments about port numbers, but I don't happen to use alternate ports daily. > Do we really need the % encoding that the URL syntax gives us? If so - > why? See above. Character set translation issues. And note that NFS URIs are coming soon to other parts of the Linux NFS administrative interface. No reason that mount.nfs should not also support them properly.
On Sun, 08 Dec 2024, Chuck Lever wrote: > On 12/7/24 3:53 PM, NeilBrown wrote: > > On Sat, 07 Dec 2024, Chuck Lever 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). > > > > Actually it isn't. You have already discussed the pub/root filehandle > > difference. > > RFC 2224 specifies both. Pub vs. root filehandles are discussed > there, along with how standard NFS URLs describe either situation. > > > > The RFC doesn't know about v4. The RFC explicitly isn't a > > standard. > > RFC 7532 contains the NFSv4 bits. RFC 2224 is not a Normative > standard, like all early NFS-related RFCs, but it is a > specification that other implementations cleave to. RFC 7532 > /is/ a Normative standard. The usage in RFC 7532 is certainly more convincing than 2224. > > > > So I wonder if this is the right approach to solve the need. > > > > What is the needed? > > Part of it seems to be non-ascii host names. Shouldn't we fix that for > > the existing syntax? What are the barriers? > > Both non-ASCII hostnames (iDNA) and export paths can contain > components with non-ASCII characters. But they cannot contain non-Unicode characters, so UTF-8 should be sufficient. > > The issue is how to specify certain code points when the client's > locale might not support them. Using a URL seems to be the mechanism > chosen by several other NFS implementations to deal with this problem. If locale-mismatch is a problem, it isn't clear to me that "mount.nfs" is the place to solve it. The problem is presented as: 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 '?'). So it sounds like the problem is copy/paste. I doubt that NFS addresses are the only things that can get corrupted. Maybe a more generic tool would be appropriate. How are people going to create the nfs: urls so they can paste them into the chat? In there a reverse tool for getting them out? Or we just just adding a hack to avoid "*PAIN* with ISPs" rather then getting the ISPs to fix their breakage? > > > > Part seems to be the inclusion of the port number. Is a "URL" really > > the best way to solve that need? > > Mount.nfs currently expects ":/" to separate host name from path. > > I think host:port:/path would be unambiguous providing "port" did not > > start "/". > > There's also IPv6 presentation addresses, which contain ":" already. Those are usually inside [] I think. > > However host:port:/path is not something that other NFS implementations > (eg that might share an automounter map) would understand. There is a > significantly higher likelihood that those implementations would be > able to interpret an NFS URI correctly. > > I'm not especially convinced by the arguments about port numbers, but > I don't happen to use alternate ports daily. > > > > Do we really need the % encoding that the URL syntax gives us? If so - > > why? > > See above. Character set translation issues. > > And note that NFS URIs are coming soon to other parts of the Linux NFS > administrative interface. No reason that mount.nfs should not also > support them properly. Are they? Where? That certainly might be a good justification. nfs: urls were introduced precisely for WebNFS (as I understand it). So when the post said "This is not about WebNFS" I had to wonder if they were really the right tool for a very different task. Maybe they are, but I think comprehensive justification is needed. Thanks, NeilBrown
On Fri, 6 Dec 2024 at 16:54, Chuck Lever <chuck.lever@oracle.com> wrote: ... > The white space below looks mangled. That needs to be fixed before > this patch can be applied. Does linux-nfs have a patchworks instance, like https://patchwork.ozlabs.org/project/uboot/patch/20241120160135.28262-1-cniedermaier@dh-electronics.com/ ? Ced
On Fri, 6 Dec 2024 at 16:54, Chuck Lever <chuck.lever@oracle.com> 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. > I think the minimum support Roland added (or what is left of it) should remain. It covers read-only ("ro=1") and read-write ("rw=1") attributes, which are clearly a property of the exported path. exportfs -U generates nfs URLs with "?ro=1" for read-only exports, and mount.nfs4 should treat such urls as the standard mandates, and not either drop an attribute (which is a violation of rfc 1738) or reject a mount request because support for "ro" parameter was dropped in this patch. ... > I wonder how distributions will test the ability to mount > percent-escaped path names, though. Maybe not upstream's problem. perl -MURI::Escape -e 'print URI::Escape::uri_escape($ARGV[0]) ; print "\n"' "money_€" money_%E2%82%AC I don't see that as a problem. Roland also has a patch for exportfs to add nfs url output with -U > > > - 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? NFSv3 is obsolete. Redhat support keeps telling us that for almost ten years now. Ced
On 12/7/24 10:29 PM, NeilBrown wrote: > On Sun, 08 Dec 2024, Chuck Lever wrote: >> On 12/7/24 3:53 PM, NeilBrown wrote: >>> On Sat, 07 Dec 2024, Chuck Lever 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). >>> >>> Actually it isn't. You have already discussed the pub/root filehandle >>> difference. >> >> RFC 2224 specifies both. Pub vs. root filehandles are discussed >> there, along with how standard NFS URLs describe either situation. >> >> >>> The RFC doesn't know about v4. The RFC explicitly isn't a >>> standard. >> >> RFC 7532 contains the NFSv4 bits. RFC 2224 is not a Normative >> standard, like all early NFS-related RFCs, but it is a >> specification that other implementations cleave to. RFC 7532 >> /is/ a Normative standard. > > The usage in RFC 7532 is certainly more convincing than 2224. > >> >> >>> So I wonder if this is the right approach to solve the need. >>> >>> What is the needed? >>> Part of it seems to be non-ascii host names. Shouldn't we fix that for >>> the existing syntax? What are the barriers? >> >> Both non-ASCII hostnames (iDNA) and export paths can contain >> components with non-ASCII characters. > > But they cannot contain non-Unicode characters, so UTF-8 should be > sufficient. > >> >> The issue is how to specify certain code points when the client's >> locale might not support them. Using a URL seems to be the mechanism >> chosen by several other NFS implementations to deal with this problem. > > If locale-mismatch is a problem, it isn't clear to me that "mount.nfs" > is the place to solve it. > > The problem is presented as: > > 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 '?'). > > So it sounds like the problem is copy/paste. I doubt that NFS addresses > are the only things that can get corrupted. > Maybe a more generic tool would be appropriate. I agree. The cited copy/paste use case is pretty weak. > How are people going to create the nfs: urls so they can paste them into > the chat? In there a reverse tool for getting them out? > > Or we just just adding a hack to avoid "*PAIN* with ISPs" rather then > getting the ISPs to fix their breakage? I tend to think our administrative interfaces could be made easier to use, over all. >>> Do we really need the % encoding that the URL syntax gives us? If so - >>> why? >> >> See above. Character set translation issues. >> >> And note that NFS URIs are coming soon to other parts of the Linux NFS >> administrative interface. No reason that mount.nfs should not also >> support them properly. > > Are they? Where? That certainly might be a good justification. The nfsref command will need to take NFS URLs in order to specify alternate port information in referrals. Tom Haynes is working on the standards part of this, I'm told. > nfs: urls were introduced precisely for WebNFS (as I understand it). So > when the post said "This is not about WebNFS" I had to wonder if they > were really the right tool for a very different task. Maybe they are, > but I think comprehensive justification is needed. I agree that the justification needs to be much stronger. The i18n stuff is complicated and needs better explanation for us mere mortals to understand. That said, support for NFS URLs is missing in Linux anyway, IMO. I don't think it hurts us to add it.
On 12/8/24 2:31 AM, Cedric Blancher wrote: > On Fri, 6 Dec 2024 at 16:54, Chuck Lever <chuck.lever@oracle.com> wrote: > ... >> The white space below looks mangled. That needs to be fixed before >> this patch can be applied. > > Does linux-nfs have a patchworks instance, like > https://patchwork.ozlabs.org/project/uboot/patch/20241120160135.28262-1-cniedermaier@dh-electronics.com/ > ? Yes, currently unused: https://patchwork.kernel.org/project/linux-nfs/list/ You can also view patches that are posted on the mailing list via the lore.kernel.org archive: https://lore.kernel.org/linux-nfs/
On 12/8/24 2:41 AM, Cedric Blancher wrote: > On Fri, 6 Dec 2024 at 16:54, Chuck Lever <chuck.lever@oracle.com> 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. >> > > I think the minimum support Roland added (or what is left of it) > should remain. It covers read-only ("ro=1") and read-write ("rw=1") > attributes, which are clearly a property of the exported path. Correct, the server mandates the security policy. However, a client can mount an RO export any way it likes. It's not an error for a client to mount an RO export with RW. Writes will fail. Mounting will not. > exportfs -U generates nfs URLs with "?ro=1" for read-only exports, and> mount.nfs4 should treat such urls as the standard mandates, and not > either drop an attribute (which is a violation of rfc 1738) or reject > a mount request because support for "ro" parameter was dropped in this > patch. The only standards language I can find regarding query components is the section of RFC 7532 I quoted here yesterday: An NFS URI MUST contain both an authority and a path component. It MUST NOT contain a query component or a fragment component. Use of the familiar "nfs" scheme name is retained. Therefore all of the parameter mechanism needs to be postponed until these details can be sorted out by standards action. Otherwise, we risk implementing something non-standard, or worse, incompatible with the standard, that will have to be maintained for a long time. This is not a NAK; rather it's a "that's not ready yet" objection. It's basically a way of getting the working parts merged sooner rather than having them wait until every last detail is worked out. Note that I will have the same objection to "exportfs -U" whenever that materializes -- it should not emit URL query components that haven't yet been standardized. You do want URL copy-and-paste between NFS implementations to work nicely, yes? >>> - 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? > > NFSv3 is obsolete. Redhat support keeps telling us that for almost ten > years now. That is wishful thinking on Red Hat's part. Hammerspace is making a business out of supporting NFSv3 data servers today, just as one example. Linux upstream still considers NFSv3 fully supported.
On Fri, 06 Dec 2024, Roland Mainz wrote: > Hi! > As Chunk is pointing out some possibe justifications for this change, I decided to have a look at the code. You email program has messed up all the spaces. If you can't find a way to configure your mail client to be sensible, attaching the patch as a text file is acceptable. This makes the patch very hard to read, so I haven't done a thorough job. The urlparser code that you have borrowed from elsewhere looks a bit clumsy. I think an '@' can only mean "username was given" if it appear in the hostname part, not after a following '/'. But that isn't checked. And we don't want user names for NFS urls. I think it would be better to write a parser from scratch which we can review and fix. It isn't that hard. See more below. > ---- > > 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, > 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 > > * Notes: > - Similar support for nfs://-URLs exists in other NFSv4.* > implementations, including Illumos, Windows ms-nfs41-client, > sahlberg/libnfs, ... > - 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 feature will not be provided for NFSv3 > > ---- snip ---- > From e7b5a3ac981727e4585288bd66d1a9b2dea045dc Mon Sep 17 00:00:00 2001 > From: Roland Mainz <roland.mainz@nrubsig.org> > 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 > Japanese, Chinese, French etc. characters) and ASCII-compatible > descriptions of NFSv4 server resources (exports). > > Reviewed-by: Martin Wege <martin.l.wege@gmail.com> > Signed-off-by: Cedric Blancher <cedric.blancher@gmail.com> > --- > utils/mount/Makefile.am | 3 +- > utils/mount/nfs4mount.c | 69 +++++++- > utils/mount/nfsmount.c | 93 ++++++++-- > utils/mount/parse_dev.c | 67 ++++++-- > utils/mount/stropts.c | 96 ++++++++++- > utils/mount/urlparser1.c | 358 +++++++++++++++++++++++++++++++++++++++ > utils/mount/urlparser1.h | 60 +++++++ > utils/mount/utils.c | 155 +++++++++++++++++ > utils/mount/utils.h | 23 +++ > 9 files changed, 887 insertions(+), 37 deletions(-) > create mode 100644 utils/mount/urlparser1.c > create mode 100644 utils/mount/urlparser1.h > > diff --git a/utils/mount/Makefile.am b/utils/mount/Makefile.am > index 83a8ee1c..0e4cab3e 100644 > --- a/utils/mount/Makefile.am > +++ b/utils/mount/Makefile.am > @@ -13,7 +13,8 @@ sbin_PROGRAMS = mount.nfs > EXTRA_DIST = nfsmount.conf $(man8_MANS) $(man5_MANS) > mount_common = error.c network.c token.c \ > parse_opt.c parse_dev.c \ > - nfsmount.c nfs4mount.c stropts.c\ > + nfsmount.c nfs4mount.c \ > + urlparser1.c urlparser1.h stropts.c \ > mount_constants.h error.h network.h token.h \ > parse_opt.h parse_dev.h \ > nfs4_mount.h stropts.h version.h \ > diff --git a/utils/mount/nfs4mount.c b/utils/mount/nfs4mount.c > index 0fe142a7..8e4fbf30 100644 > --- a/utils/mount/nfs4mount.c > +++ b/utils/mount/nfs4mount.c > @@ -50,8 +50,10 @@ > #include "mount_constants.h" > #include "nfs4_mount.h" > #include "nfs_mount.h" > +#include "urlparser1.h" > #include "error.h" > #include "network.h" > +#include "utils.h" > > #if defined(VAR_LOCK_DIR) > #define DEFAULT_DIR VAR_LOCK_DIR > @@ -182,7 +184,7 @@ int nfs4mount(const char *spec, const char *node, int flags, > int num_flavour = 0; > int ip_addr_in_opts = 0; > > - char *hostname, *dirname, *old_opts; > + char *hostname, *dirname, *mb_dirname = NULL, *old_opts; > char new_opts[1024]; > char *opt, *opteq; > char *s; > @@ -192,15 +194,66 @@ int nfs4mount(const char *spec, const char > *node, int flags, > int retry; > int retval = EX_FAIL; > time_t timeout, t; > + int nfs_port = NFS_PORT; > + parsed_nfs_url pnu; Please don't use typedef for structs. This should be " struct parsed_nfs_url > + > + (void)memset(&pnu, 0, sizeof(parsed_nfs_url)); memset(&pnu, 0, sizeof(pnu)) is preferred. > > if (strlen(spec) >= sizeof(hostdir)) { > nfs_error(_("%s: excessively long host:dir argument\n"), > progname); > goto fail; > } > - strcpy(hostdir, spec); > - if (parse_devname(hostdir, &hostname, &dirname)) > - goto fail; > + > + /* > + * Support nfs://-URLS per RFC 2224 ("NFS URL > + * SCHEME", see https://www.rfc-editor.org/rfc/rfc2224.html), > + * including custom port (nfs://hostname@port/path/...) > + * and URL parameter (e.g. nfs://.../?param1=val1¶m2=val2 > + * support Please mention RFC7532 as ell. Also the port should be separated by a ':', not '@' > + */ > + if (is_spec_nfs_url(spec)) { > + if (!mount_parse_nfs_url(spec, &pnu)) { > + goto fail; > + } is_spec_nfs_url() is (almost) always followed by mount_parse_nfs_url(). I think it would be best to combine the two. > + > + /* > + * |pnu.uctx->path| is in UTF-8, but we need the data > + * in the current local locale's encoding, as mount(2) > + * does not have something like a |MS_UTF8_SPEC| flag > + * to indicate that the input path is in UTF-8, > + * independently of the current locale > + */ I don't understand this comment at all. mount(2) doesn't care about locale (as far as I know). The "source" is simply a string of bytes that it is up to the filesystem to interpret. NFS will always interpret it as utf8. So no conversion is needed. > + hostname = pnu.uctx->hostport.hostname; > + dirname = mb_dirname = utf8str2mbstr(pnu.uctx->path); > + > + (void)snprintf(hostdir, sizeof(hostdir), "%s:/%s", > + hostname, dirname); > + spec = hostdir; > + > + if (pnu.uctx->hostport.port != -1) { > + nfs_port = pnu.uctx->hostport.port; > + } > + > + /* > + * Values added here based on URL parameters > + * should be added the front of the list of options, > + * so users can override the nfs://-URL given default. > + * > + * FIXME: We do not do that here for |MS_RDONLY|! > + */ > + if (pnu.mount_params.read_only != TRIS_BOOL_NOT_SET) { > + if (pnu.mount_params.read_only) > + flags |= MS_RDONLY; > + else > + flags &= ~MS_RDONLY; > + } > + } else { > + (void)strcpy(hostdir, spec); I'm not paying much attention to the "parameter" code. I think more justification is needed before we can consider it. > + > + if (parse_devname(hostdir, &hostname, &dirname)) > + goto fail; > + } > > if (fill_ipv4_sockaddr(hostname, &server_addr)) > goto fail; > @@ -247,7 +300,7 @@ int nfs4mount(const char *spec, const char *node, int flags, > /* > * NFSv4 specifies that the default port should be 2049 > */ > - server_addr.sin_port = htons(NFS_PORT); > + server_addr.sin_port = htons(nfs_port); > > /* parse options */ > > @@ -474,8 +527,14 @@ int nfs4mount(const char *spec, const char *node, > int flags, > } > } > > + mount_free_parse_nfs_url(&pnu); > + free(mb_dirname); > + > return EX_SUCCESS; > > fail: > + mount_free_parse_nfs_url(&pnu); > + free(mb_dirname); > + > return retval; > } > diff --git a/utils/mount/nfsmount.c b/utils/mount/nfsmount.c > index a1c92fe8..e61d718a 100644 > --- a/utils/mount/nfsmount.c > +++ b/utils/mount/nfsmount.c > @@ -63,11 +63,13 @@ > #include "xcommon.h" > #include "mount.h" > #include "nfs_mount.h" > +#include "urlparser1.h" > #include "mount_constants.h" > #include "nls.h" > #include "error.h" > #include "network.h" > #include "version.h" > +#include "utils.h" > > #ifdef HAVE_RPCSVC_NFS_PROT_H > #include <rpcsvc/nfs_prot.h> > @@ -493,7 +495,7 @@ nfsmount(const char *spec, const char *node, int flags, > char **extra_opts, int fake, int running_bg) > { > char hostdir[1024]; > - char *hostname, *dirname, *old_opts, *mounthost = NULL; > + char *hostname, *dirname, *mb_dirname = NULL, *old_opts, *mounthost = NULL; > char new_opts[1024], cbuf[1024]; > static struct nfs_mount_data data; > int val; > @@ -521,29 +523,79 @@ nfsmount(const char *spec, const char *node, int flags, > time_t t; > time_t prevt; > time_t timeout; > + int nfsurl_port = -1; > + parsed_nfs_url pnu; > + > + (void)memset(&pnu, 0, sizeof(parsed_nfs_url)); > > if (strlen(spec) >= sizeof(hostdir)) { > nfs_error(_("%s: excessively long host:dir argument"), > progname); > goto fail; > } > - strcpy(hostdir, spec); > - if ((s = strchr(hostdir, ':'))) { > - hostname = hostdir; > - dirname = s + 1; > - *s = '\0'; > - /* Ignore all but first hostname in replicated mounts > - until they can be fully supported. (mack@sgi.com) */ > - if ((s = strchr(hostdir, ','))) { > + > + /* > + * Support nfs://-URLS per RFC 2224 ("NFS URL > + * SCHEME", see https://www.rfc-editor.org/rfc/rfc2224.html), > + * including custom port (nfs://hostname@port/path/...) > + * and URL parameter (e.g. nfs://.../?param1=val1¶m2=val2 > + * support > + */ > + if (is_spec_nfs_url(spec)) { > + if (!mount_parse_nfs_url(spec, &pnu)) { > + goto fail; > + } > + > + /* > + * |pnu.uctx->path| is in UTF-8, but we need the data > + * in the current local locale's encoding, as mount(2) > + * does not have something like a |MS_UTF8_SPEC| flag > + * to indicate that the input path is in UTF-8, > + * independently of the current locale > + */ > + hostname = pnu.uctx->hostport.hostname; > + dirname = mb_dirname = utf8str2mbstr(pnu.uctx->path); > + > + (void)snprintf(hostdir, sizeof(hostdir), "%s:/%s", > + hostname, dirname); > + spec = hostdir; > + > + if (pnu.uctx->hostport.port != -1) { > + nfsurl_port = pnu.uctx->hostport.port; > + } > + > + /* > + * Values added here based on URL parameters > + * should be added the front of the list of options, > + * so users can override the nfs://-URL given default. > + * > + * FIXME: We do not do that here for |MS_RDONLY|! > + */ > + if (pnu.mount_params.read_only != TRIS_BOOL_NOT_SET) { > + if (pnu.mount_params.read_only) > + flags |= MS_RDONLY; > + else > + flags &= ~MS_RDONLY; > + } > + } else { > + (void)strcpy(hostdir, spec); > + if ((s = strchr(hostdir, ':'))) { > + hostname = hostdir; > + dirname = s + 1; > *s = '\0'; > - nfs_error(_("%s: warning: " > - "multiple hostnames not supported"), > + /* Ignore all but first hostname in replicated mounts > + until they can be fully supported. (mack@sgi.com) */ > + if ((s = strchr(hostdir, ','))) { > + *s = '\0'; > + nfs_error(_("%s: warning: " > + "multiple hostnames not supported"), > progname); > - } > - } else { > - nfs_error(_("%s: directory to mount not in host:dir format"), > + } > + } else { > + nfs_error(_("%s: directory to mount not in host:dir format"), > progname); > - goto fail; > + goto fail; > + } > } > > if (!nfs_gethostbyname(hostname, nfs_saddr)) > @@ -579,6 +631,14 @@ nfsmount(const char *spec, const char *node, int flags, > memset(nfs_pmap, 0, sizeof(*nfs_pmap)); > nfs_pmap->pm_prog = NFS_PROGRAM; > > + if (nfsurl_port != -1) { > + /* > + * Set custom TCP port defined by a nfs://-URL here, > + * so $ mount -o port ... # can be used to override > + */ > + nfs_pmap->pm_port = nfsurl_port; > + } > + > /* parse options */ > new_opts[0] = 0; > if (!parse_options(old_opts, &data, &bg, &retry, &mnt_server, &nfs_server, > @@ -863,10 +923,13 @@ noauth_flavors: > } > } > > + mount_free_parse_nfs_url(&pnu); > + > return EX_SUCCESS; > > /* abort */ > fail: > + mount_free_parse_nfs_url(&pnu); > if (fsock != -1) > close(fsock); > return retval; > diff --git a/utils/mount/parse_dev.c b/utils/mount/parse_dev.c > index 2ade5d5d..d9f8cf59 100644 > --- a/utils/mount/parse_dev.c > +++ b/utils/mount/parse_dev.c > @@ -27,6 +27,8 @@ > #include "xcommon.h" > #include "nls.h" > #include "parse_dev.h" > +#include "urlparser1.h" > +#include "utils.h" > > #ifndef NFS_MAXHOSTNAME > #define NFS_MAXHOSTNAME (255) > @@ -179,17 +181,62 @@ static int nfs_parse_square_bracket(const char *dev, > } > > /* > - * RFC 2224 says an NFS client must grok "public file handles" to > - * support NFS URLs. Linux doesn't do that yet. Print a somewhat > - * helpful error message in this case instead of pressing forward > - * with the mount request and failing with a cryptic error message > - * later. > + * Support nfs://-URLS per RFC 2224 ("NFS URL > + * SCHEME", see https://www.rfc-editor.org/rfc/rfc2224.html), > + * including port support (nfs://hostname@port/path/...) Note that the above example is a "public" address. We don't support those in Linux - only the root. So you need to match nfs://hostname:port//path.... i.e. a double slash at the start of the path. I don't think your code does that. > */ > -static int nfs_parse_nfs_url(__attribute__((unused)) const char *dev, > - __attribute__((unused)) char **hostname, > - __attribute__((unused)) char **pathname) > +static int nfs_parse_nfs_url(const char *dev, > + char **out_hostname, > + char **out_pathname) > { > - nfs_error(_("%s: NFS URLs are not supported"), progname); > + parsed_nfs_url pnu; > + > + if (out_hostname) > + *out_hostname = NULL; > + if (out_pathname) > + *out_pathname = NULL; > + > + /* > + * Support nfs://-URLS per RFC 2224 ("NFS URL > + * SCHEME", see https://www.rfc-editor.org/rfc/rfc2224.html), > + * including custom port (nfs://hostname@port/path/...) > + * and URL parameter (e.g. nfs://.../?param1=val1¶m2=val2 > + * support > + */ > + if (!mount_parse_nfs_url(dev, &pnu)) { > + goto fail; > + } > + > + if (pnu.uctx->hostport.port != -1) { > + /* NOP here, unless we switch from hostname to hostport */ > + } > + > + if (out_hostname) > + *out_hostname = strdup(pnu.uctx->hostport.hostname); > + if (out_pathname) > + *out_pathname = utf8str2mbstr(pnu.uctx->path); > + > + if (((out_hostname)?(*out_hostname == NULL):0) || > + ((out_pathname)?(*out_pathname == NULL):0)) { > + nfs_error(_("%s: out of memory"), > + progname); > + goto fail; > + } > + > + mount_free_parse_nfs_url(&pnu); > + > + return 1; > + > +fail: > + mount_free_parse_nfs_url(&pnu); > + if (out_hostname) { > + free(*out_hostname); > + *out_hostname = NULL; > + } > + if (out_pathname) { > + free(*out_pathname); > + *out_pathname = NULL; > + } > return 0; > } > > @@ -223,7 +270,7 @@ int nfs_parse_devname(const char *devname, > return nfs_pdn_nomem_err(); > if (*dev == '[') > result = nfs_parse_square_bracket(dev, hostname, pathname); > - else if (strncmp(dev, "nfs://", 6) == 0) > + else if (is_spec_nfs_url(dev)) > result = nfs_parse_nfs_url(dev, hostname, pathname); > else > result = nfs_parse_simple_hostname(dev, hostname, pathname); > diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c > index 23f0a8c0..ad92ab78 100644 > --- a/utils/mount/stropts.c > +++ b/utils/mount/stropts.c > @@ -42,6 +42,7 @@ > #include "nls.h" > #include "nfsrpc.h" > #include "mount_constants.h" > +#include "urlparser1.h" > #include "stropts.h" > #include "error.h" > #include "network.h" > @@ -50,6 +51,7 @@ > #include "parse_dev.h" > #include "conffile.h" > #include "misc.h" > +#include "utils.h" > > #ifndef NFS_PROGRAM > #define NFS_PROGRAM (100003) > @@ -643,24 +645,106 @@ static int nfs_sys_mount(struct nfsmount_info > *mi, struct mount_options *opts) > { > char *options = NULL; > int result; > + int nfs_port = 2049; > > if (mi->fake) > return 1; > > - if (po_join(opts, &options) == PO_FAILED) { > - errno = EIO; > - return 0; > - } > + /* > + * Support nfs://-URLS per RFC 2224 ("NFS URL > + * SCHEME", see https://www.rfc-editor.org/rfc/rfc2224.html), > + * including custom port (nfs://hostname@port/path/...) > + * and URL parameter (e.g. nfs://.../?param1=val1¶m2=val2 > + * support > + */ > + if (is_spec_nfs_url(mi->spec)) { > + parsed_nfs_url pnu; > + char *mb_path; > + char mount_source[1024]; > + > + if (!mount_parse_nfs_url(mi->spec, &pnu)) { > + result = 1; > + errno = EINVAL; > + goto done; > + } > + > + /* > + * |pnu.uctx->path| is in UTF-8, but we need the data > + * in the current local locale's encoding, as mount(2) > + * does not have something like a |MS_UTF8_SPEC| flag > + * to indicate that the input path is in UTF-8, > + * independently of the current locale > + */ > + mb_path = utf8str2mbstr(pnu.uctx->path); > + if (!mb_path) { > + nfs_error(_("%s: Could not convert path to local encoding."), > + progname); > + mount_free_parse_nfs_url(&pnu); > + result = 1; > + errno = EINVAL; > + goto done; > + } > + > + (void)snprintf(mount_source, sizeof(mount_source), > + "%s:/%s", > + pnu.uctx->hostport.hostname, > + mb_path); > + free(mb_path); > + > + if (pnu.uctx->hostport.port != -1) { > + nfs_port = pnu.uctx->hostport.port; > + } > > - result = mount(mi->spec, mi->node, mi->type, > + /* > + * Insert "port=" option with the value from the nfs:// > + * URL at the beginning of the list of options, so > + * users can override it with $ mount.nfs4 -o port= #, > + * e.g. > + * $ mount.nfs4 -o port=1234 nfs://10.49.202.230:400//bigdisk /mnt4 # > + * should use port 1234, and not port 400 as specified > + * in the URL. > + */ > + char portoptbuf[5+32+1]; > + (void)snprintf(portoptbuf, sizeof(portoptbuf), "port=%d", nfs_port); > + (void)po_insert(opts, portoptbuf); > + > + if (pnu.mount_params.read_only != TRIS_BOOL_NOT_SET) { > + if (pnu.mount_params.read_only) > + mi->flags |= MS_RDONLY; > + else > + mi->flags &= ~MS_RDONLY; > + } > + > + mount_free_parse_nfs_url(&pnu); > + > + if (po_join(opts, &options) == PO_FAILED) { > + errno = EIO; > + result = 1; > + goto done; > + } > + > + result = mount(mount_source, mi->node, mi->type, > + mi->flags & ~(MS_USER|MS_USERS), options); > + free(options); > + } else { > + if (po_join(opts, &options) == PO_FAILED) { > + errno = EIO; > + result = 1; > + goto done; > + } > + > + result = mount(mi->spec, mi->node, mi->type, > mi->flags & ~(MS_USER|MS_USERS), options); > - free(options); > + free(options); > + } > > if (verbose && result) { > int save = errno; > nfs_error(_("%s: mount(2): %s"), progname, strerror(save)); > errno = save; > } > + > +done: > return !result; > } > > diff --git a/utils/mount/urlparser1.c b/utils/mount/urlparser1.c > new file mode 100644 > index 00000000..d4c6f339 > --- /dev/null > +++ b/utils/mount/urlparser1.c > @@ -0,0 +1,358 @@ > +/* > + * MIT License > + * > + * Copyright (c) 2024 Roland Mainz <roland.mainz@nrubsig.org> > + * > + * Permission is hereby granted, free of charge, to any person obtaining a copy > + * of this software and associated documentation files (the > "Software"), to deal > + * in the Software without restriction, including without limitation the rights > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell > + * copies of the Software, and to permit persons to whom the Software is > + * furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be > included in all > + * copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE > + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > ARISING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > DEALINGS IN THE > + * SOFTWARE. > + */ > + > +/* urlparser1.c - simple URL parser */ > + > +#include <stdlib.h> > +#include <stdbool.h> > +#include <string.h> > +#include <ctype.h> > +#include <stdio.h> > + > +#ifdef DBG_USE_WIDECHAR > +#include <wchar.h> > +#include <locale.h> > +#include <io.h> > +#include <fcntl.h> > +#endif /* DBG_USE_WIDECHAR */ > + > +#include "urlparser1.h" > + > +typedef struct _url_parser_context_private { > + url_parser_context c; > + > + /* Private data */ > + char *parameter_string_buff; > +} url_parser_context_private; > + > +#define MAX_URL_PARAMETERS 256 > + > +/* > + * Original extended regular expression: > + * > + * "^" > + * "(.+?)" // scheme > + * "://" // '://' > + * "(" // login > + * "(?:" > + * "(.+?)" // user (optional) > + * "(?::(.+))?" // password (optional) > + * "@" > + * ")?" > + * "(" // hostport > + * "(.+?)" // host > + * "(?::([[:digit:]]+))?" // port (optional) > + * ")" > + * ")" > + * "(?:/(.*?))?" // path (optional) > + * "(?:\?(.*?))?" // URL parameters (optional) > + * "$" > + */ > + > +#define DBGNULLSTR(s) (((s)!=NULL)?(s):"<NULL>") > +#if 0 || defined(TEST_URLPARSER) > +#define D(x) x > +#else > +#define D(x) > +#endif > + > +#ifdef DBG_USE_WIDECHAR > +/* > + * Use wide-char APIs on WIN32, otherwise we cannot output > + * Japanese/Chinese/etc correctly > + */ > +#define DBG_PUTS(str, fp) fputws(L"" str, (fp)) > +#define DBG_PUTC(c, fp) fputwc(btowc(c), (fp)) > +#define DBG_PRINTF(fp, fmt, ...) fwprintf((fp), L"" fmt, __VA_ARGS__) > +#else > +#define DBG_PUTS(str, fp) fputs((str), (fp)) > +#define DBG_PUTC(c, fp) fputc((c), (fp)) > +#define DBG_PRINTF(fp, fmt, ...) fprintf((fp), fmt, __VA_ARGS__) > +#endif /* DBG_USE_WIDECHAR */ > + > +static > +void urldecodestr(char *outbuff, const char *buffer, size_t len) > +{ > + size_t i, j; > + > + for (i = j = 0 ; i < len ; ) { > + switch (buffer[i]) { > + case '%': > + if ((i + 2) < len) { > + if (isxdigit((int)buffer[i+1]) && isxdigit((int)buffer[i+2])) { You don't need 2 "if"s here. if (i+2 < len && isxdigit(buffer[i+1]) && isxdigit(buffer[i+2])) { > + const char hexstr[3] = { > + buffer[i+1], > + buffer[i+2], > + '\0' > + }; > + outbuff[j++] = (unsigned char)strtol(hexstr, NULL, 16); > + i += 3; > + } else { > + /* invalid hex digit */ > + outbuff[j++] = buffer[i]; > + i++; > + } > + } else { > + /* incomplete hex digit */ > + outbuff[j++] = buffer[i]; > + i++; > + } > + break; > + case '+': > + outbuff[j++] = ' '; > + i++; > + break; > + default: > + outbuff[j++] = buffer[i++]; > + break; > + } > + } > + > + outbuff[j] = '\0'; > +} > + > +url_parser_context *url_parser_create_context(const char *in_url, > unsigned int flags) > +{ > + url_parser_context_private *uctx; > + char *s; > + size_t in_url_len; > + size_t context_len; > + > + /* |flags| is for future extensions */ > + (void)flags; > + > + if (!in_url) > + return NULL; > + > + in_url_len = strlen(in_url); > + > + context_len = sizeof(url_parser_context_private) + > + ((in_url_len+1)*6) + > + (sizeof(url_parser_name_value)*MAX_URL_PARAMETERS)+sizeof(void*); > + uctx = malloc(context_len); > + if (!uctx) > + return NULL; > + > + s = (void *)(uctx+1); > + uctx->c.in_url = s; s+= in_url_len+1; > + (void)strcpy(uctx->c.in_url, in_url); > + uctx->c.scheme = s; s+= in_url_len+1; > + uctx->c.login.username = s; s+= in_url_len+1; > + uctx->c.hostport.hostname = s; s+= in_url_len+1; > + uctx->c.path = s; s+= in_url_len+1; > + uctx->c.hostport.port = -1; > + uctx->c.num_parameters = -1; > + uctx->c.parameters = (void *)s; s+= > (sizeof(url_parser_name_value)*MAX_URL_PARAMETERS)+sizeof(void*); > + uctx->parameter_string_buff = s; s+= in_url_len+1; > + > + return &uctx->c; > +} > + > +int url_parser_parse(url_parser_context *ctx) > +{ > + url_parser_context_private *uctx = (url_parser_context_private *)ctx; > + > + D((void)DBG_PRINTF(stderr, "## parser in_url='%s'\n", uctx->c.in_url)); > + > + char *s; > + const char *urlstr = uctx->c.in_url; > + size_t slen; > + > + s = strstr(urlstr, "://"); > + if (!s) { > + D((void)DBG_PUTS("url_parser: Not an URL\n", stderr)); > + return -1; > + } > + > + slen = s-urlstr; > + (void)memcpy(uctx->c.scheme, urlstr, slen); > + uctx->c.scheme[slen] = '\0'; > + urlstr += slen + 3; > + > + D((void)DBG_PRINTF(stdout, "scheme='%s', rest='%s'\n", > uctx->c.scheme, urlstr)); > + > + s = strstr(urlstr, "@"); > + if (s) { > + /* URL has user/password */ This need to check the '@' is after a '/'. > + slen = s-urlstr; > + urldecodestr(uctx->c.login.username, urlstr, slen); > + urlstr += slen + 1; > + > + s = strstr(uctx->c.login.username, ":"); > + if (s) { > + /* found passwd */ > + uctx->c.login.passwd = s+1; > + *s = '\0'; > + } > + else { > + uctx->c.login.passwd = NULL; > + } > + > + /* catch password-only URLs */ > + if (uctx->c.login.username[0] == '\0') > + uctx->c.login.username = NULL; > + } > + else { > + uctx->c.login.username = NULL; > + uctx->c.login.passwd = NULL; > + } > + > + D((void)DBG_PRINTF(stdout, "login='%s', passwd='%s', rest='%s'\n", > + DBGNULLSTR(uctx->c.login.username), > + DBGNULLSTR(uctx->c.login.passwd), > + DBGNULLSTR(urlstr))); > + > + char *raw_parameters; > + > + uctx->c.num_parameters = 0; > + raw_parameters = strstr(urlstr, "?"); > + /* Do we have a non-empty parameter string ? */ > + if (raw_parameters && (raw_parameters[1] != '\0')) { > + *raw_parameters++ = '\0'; > + D((void)DBG_PRINTF(stdout, "raw parameters = '%s'\n", raw_parameters)); > + > + char *ps = raw_parameters; > + char *pv; /* parameter value */ > + char *na; /* next '&' */ > + char *pb = uctx->parameter_string_buff; > + char *pname; > + char *pvalue; > + ssize_t pi; > + > + for (pi = 0; pi < MAX_URL_PARAMETERS ; pi++) { > + pname = ps; > + > + /* > + * Handle parameters without value, > + * e.g. "path?name1&name2=value2" > + */ > + na = strstr(ps, "&"); > + pv = strstr(ps, "="); > + if (pv && (na?(na > pv):true)) { > + *pv++ = '\0'; > + pvalue = pv; > + ps = pv; > + } > + else { > + pvalue = NULL; > + } > + > + if (na) { > + *na++ = '\0'; > + } > + > + /* URLDecode parameter name */ > + urldecodestr(pb, pname, strlen(pname)); > + uctx->c.parameters[pi].name = pb; > + pb += strlen(uctx->c.parameters[pi].name)+1; > + > + /* URLDecode parameter value */ > + if (pvalue) { > + urldecodestr(pb, pvalue, strlen(pvalue)); > + uctx->c.parameters[pi].value = pb; > + pb += strlen(uctx->c.parameters[pi].value)+1; > + } > + else { > + uctx->c.parameters[pi].value = NULL; > + } > + > + /* Next '&' ? */ > + if (!na) > + break; > + > + ps = na; > + } > + > + uctx->c.num_parameters = pi+1; > + } > + > + s = strstr(urlstr, "/"); > + if (s) { > + /* URL has hostport */ > + slen = s-urlstr; > + urldecodestr(uctx->c.hostport.hostname, urlstr, slen); > + urlstr += slen + 1; > + > + /* > + * check for addresses within '[' and ']', like > + * IPv6 addresses > + */ > + s = uctx->c.hostport.hostname; > + if (s[0] == '[') > + s = strstr(s, "]"); > + > + if (s == NULL) { > + D((void)DBG_PUTS("url_parser: Unmatched '[' in hostname\n", stderr)); > + return -1; > + } > + > + s = strstr(s, ":"); > + if (s) { > + /* found port number */ > + uctx->c.hostport.port = atoi(s+1); > + *s = '\0'; > + } > + } > + else { > + (void)strcpy(uctx->c.hostport.hostname, urlstr); > + uctx->c.path = NULL; > + urlstr = NULL; > + } > + > + D( > + (void)DBG_PRINTF(stdout, > + "hostport='%s', port=%d, rest='%s', num_parameters=%d\n", > + DBGNULLSTR(uctx->c.hostport.hostname), > + uctx->c.hostport.port, > + DBGNULLSTR(urlstr), > + (int)uctx->c.num_parameters); > + ); > + > + > + D( > + ssize_t dpi; > + for (dpi = 0 ; dpi < uctx->c.num_parameters ; dpi++) { > + (void)DBG_PRINTF(stdout, > + "param[%d]: name='%s'/value='%s'\n", > + (int)dpi, > + uctx->c.parameters[dpi].name, > + DBGNULLSTR(uctx->c.parameters[dpi].value)); > + } > + ); > + > + if (!urlstr) { > + goto done; > + } > + > + urldecodestr(uctx->c.path, urlstr, strlen(urlstr)); > + D((void)DBG_PRINTF(stdout, "path='%s'\n", uctx->c.path)); > + > +done: > + return 0; > +} > + > +void url_parser_free_context(url_parser_context *c) > +{ > + free(c); > +} > diff --git a/utils/mount/urlparser1.h b/utils/mount/urlparser1.h > new file mode 100644 > index 00000000..515eea9d > --- /dev/null > +++ b/utils/mount/urlparser1.h > @@ -0,0 +1,60 @@ > +/* > + * MIT License > + * > + * Copyright (c) 2024 Roland Mainz <roland.mainz@nrubsig.org> > + * > + * Permission is hereby granted, free of charge, to any person obtaining a copy > + * of this software and associated documentation files (the > "Software"), to deal > + * in the Software without restriction, including without limitation the rights > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell > + * copies of the Software, and to permit persons to whom the Software is > + * furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be > included in all > + * copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE > + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > ARISING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > DEALINGS IN THE > + * SOFTWARE. > + */ > + > +/* urlparser1.h - header for simple URL parser */ > + > +#ifndef __URLPARSER1_H__ > +#define __URLPARSER1_H__ 1 > + > +#include <stdlib.h> > + > +typedef struct _url_parser_name_value { > + char *name; > + char *value; > +} url_parser_name_value; > + > +typedef struct _url_parser_context { > + char *in_url; > + > + char *scheme; > + struct { > + char *username; > + char *passwd; > + } login; > + struct { > + char *hostname; > + signed int port; > + } hostport; > + char *path; > + > + ssize_t num_parameters; > + url_parser_name_value *parameters; > +} url_parser_context; > + > +/* Prototypes */ > +url_parser_context *url_parser_create_context(const char *in_url, > unsigned int flags); > +int url_parser_parse(url_parser_context *uctx); > +void url_parser_free_context(url_parser_context *c); > + > +#endif /* !__URLPARSER1_H__ */ > diff --git a/utils/mount/utils.c b/utils/mount/utils.c > index b7562a47..2d4cfa5a 100644 > --- a/utils/mount/utils.c > +++ b/utils/mount/utils.c > @@ -28,6 +28,7 @@ > #include <unistd.h> > #include <sys/types.h> > #include <sys/stat.h> > +#include <iconv.h> > > #include "sockaddr.h" > #include "nfs_mount.h" > @@ -173,3 +174,157 @@ int nfs_umount23(const char *devname, char *string) > free(dirname); > return result; > } > + > +/* Convert UTF-8 string to multibyte string in the current locale */ > +char *utf8str2mbstr(const char *utf8_str) > +{ > + iconv_t cd; > + > + cd = iconv_open("UTF-8", ""); > + if (cd == (iconv_t)-1) { > + perror("utf8str2mbstr: iconv_open failed"); > + return NULL; > + } > + > + size_t inbytesleft = strlen(utf8_str); > + char *inbuf = (char *)utf8_str; > + size_t outbytesleft = inbytesleft*4+1; > + char *outbuf = malloc(outbytesleft); > + char *outbuf_orig = outbuf; > + > + if (!outbuf) { > + perror("utf8str2mbstr: out of memory"); > + (void)iconv_close(cd); > + return NULL; > + } > + > + int ret = iconv(cd, &inbuf, &inbytesleft, &outbuf, &outbytesleft); > + if (ret == -1) { > + perror("utf8str2mbstr: iconv() failed"); > + free(outbuf_orig); > + (void)iconv_close(cd); > + return NULL; > + } > + > + *outbuf = '\0'; > + > + (void)iconv_close(cd); > + return outbuf_orig; > +} > + > +/* fixme: We should use |bool|! */ > +int is_spec_nfs_url(const char *spec) > +{ > + return (!strncmp(spec, "nfs://", 6)); Ugh. I personally hate this !strcmp style. It looks like is testing for "not" something, but it isn't. I MUCH prefer strncmp(spec, "nfs://", 6) == 0 The "==" makes it clear that it is testing for equality. > +} > + > +int mount_parse_nfs_url(const char *spec, parsed_nfs_url *pnu) > +{ > + int result = 1; > + url_parser_context *uctx = NULL; > + > + (void)memset(pnu, 0, sizeof(parsed_nfs_url)); > + pnu->mount_params.read_only = TRIS_BOOL_NOT_SET; > + > + uctx = url_parser_create_context(spec, 0); > + if (!uctx) { > + nfs_error(_("%s: out of memory"), > + progname); > + result = 1; > + goto done; > + } > + > + if (url_parser_parse(uctx) < 0) { > + nfs_error(_("%s: Error parsing nfs://-URL."), > + progname); > + result = 1; > + goto done; > + } > + if (uctx->login.username || uctx->login.passwd) { > + nfs_error(_("%s: Username/Password are not defined for nfs://-URL."), > + progname); > + result = 1; > + goto done; > + } > + if (!uctx->path) { > + nfs_error(_("%s: Path missing in nfs://-URL."), > + progname); > + result = 1; > + goto done; > + } > + if (uctx->path[0] != '/') { > + nfs_error(_("%s: Relative nfs://-URLs are not supported."), > + progname); > + result = 1; > + goto done; > + } > + > + if (uctx->num_parameters > 0) { > + int pi; > + const char *pname; > + const char *pvalue; > + > + /* > + * Values added here based on URL parameters > + * should be added the front of the list of options, > + * so users can override the nfs://-URL given default. > + */ > + for (pi = 0; pi < uctx->num_parameters ; pi++) { > + pname = uctx->parameters[pi].name; > + pvalue = uctx->parameters[pi].value; > + > + if (!strcmp(pname, "rw")) { > + if ((pvalue == NULL) || (!strcmp(pvalue, "1"))) { > + pnu->mount_params.read_only = TRIS_BOOL_FALSE; > + } > + else if (!strcmp(pvalue, "0")) { > + pnu->mount_params.read_only = TRIS_BOOL_TRUE; > + } > + else { > + nfs_error(_("%s: Unsupported nfs://-URL " > + "parameter '%s' value '%s'."), > + progname, pname, pvalue); > + result = 1; > + goto done; > + } > + } > + else if (!strcmp(pname, "ro")) { > + if ((pvalue == NULL) || (!strcmp(pvalue, "1"))) { > + pnu->mount_params.read_only = TRIS_BOOL_TRUE; > + } > + else if (!strcmp(pvalue, "0")) { > + pnu->mount_params.read_only = TRIS_BOOL_FALSE; > + } > + else { > + nfs_error(_("%s: Unsupported nfs://-URL " > + "parameter '%s' value '%s'."), > + progname, pname, pvalue); > + result = 1; > + goto done; > + } > + } > + else { > + nfs_error(_("%s: Unsupported nfs://-URL " > + "parameter '%s'."), > + progname, pname); > + result = 1; > + goto done; > + } > + } > + } > + > + result = 0; > +done: > + if (result != 0) { > + url_parser_free_context(uctx); > + return 0; > + } > + > + pnu->uctx = uctx; > + return 1; > +} > + > +void mount_free_parse_nfs_url(parsed_nfs_url *pnu) > +{ > + url_parser_free_context(pnu->uctx); > +} > diff --git a/utils/mount/utils.h b/utils/mount/utils.h > index 224918ae..465c0a5e 100644 > --- a/utils/mount/utils.h > +++ b/utils/mount/utils.h > @@ -24,13 +24,36 @@ > #define _NFS_UTILS_MOUNT_UTILS_H > > #include "parse_opt.h" > +#include "urlparser1.h" > > +/* Boolean with three states: { not_set, false, true */ > +typedef signed char tristate_bool; > +#define TRIS_BOOL_NOT_SET (-1) > +#define TRIS_BOOL_TRUE (1) > +#define TRIS_BOOL_FALSE (0) > + > +#define TRIS_BOOL_GET_VAL(tsb, tsb_default) \ > + (((tsb)!=TRIS_BOOL_NOT_SET)?(tsb):(tsb_default)) > + > +typedef struct _parsed_nfs_url { > + url_parser_context *uctx; > + struct { > + tristate_bool read_only; > + } mount_params; > +} parsed_nfs_url; > + > +/* Prototypes */ > int discover_nfs_mount_data_version(int *string_ver); > void print_one(char *spec, char *node, char *type, char *opts); > void mount_usage(void); > void umount_usage(void); > int chk_mountpoint(const char *mount_point); > +char *utf8str2mbstr(const char *utf8_str); > +int is_spec_nfs_url(const char *spec); > > int nfs_umount23(const char *devname, char *string); > > +int mount_parse_nfs_url(const char *spec, parsed_nfs_url *pnu); > +void mount_free_parse_nfs_url(parsed_nfs_url *pnu); > + > #endif /* !_NFS_UTILS_MOUNT_UTILS_H */ > -- > 2.30.2 > ---- snip ---- > > ---- > > Bye, > Roland > -- > __ . . __ > (o.\ \/ /.o) roland.mainz@nrubsig.org > \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer > /O /==\ O\ TEL +49 641 3992797 > (;O/ \/ \O;) > NeilBrown
On Sun, Dec 8, 2024 at 4:58 PM Chuck Lever <chuck.lever@oracle.com> wrote: > > On 12/7/24 10:29 PM, NeilBrown wrote: > > On Sun, 08 Dec 2024, Chuck Lever wrote: > >> On 12/7/24 3:53 PM, NeilBrown wrote: > >>> On Sat, 07 Dec 2024, Chuck Lever 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). > >>> > >>> Actually it isn't. You have already discussed the pub/root filehandle > >>> difference. > >> > >> RFC 2224 specifies both. Pub vs. root filehandles are discussed > >> there, along with how standard NFS URLs describe either situation. > >> > >> > >>> The RFC doesn't know about v4. The RFC explicitly isn't a > >>> standard. > >> > >> RFC 7532 contains the NFSv4 bits. RFC 2224 is not a Normative > >> standard, like all early NFS-related RFCs, but it is a > >> specification that other implementations cleave to. RFC 7532 > >> /is/ a Normative standard. > > > > The usage in RFC 7532 is certainly more convincing than 2224. > > > >> > >> > >>> So I wonder if this is the right approach to solve the need. > >>> > >>> What is the needed? > >>> Part of it seems to be non-ascii host names. Shouldn't we fix that for > >>> the existing syntax? What are the barriers? > >> > >> Both non-ASCII hostnames (iDNA) and export paths can contain > >> components with non-ASCII characters. > > > > But they cannot contain non-Unicode characters, so UTF-8 should be > > sufficient. > > > >> > >> The issue is how to specify certain code points when the client's > >> locale might not support them. Using a URL seems to be the mechanism > >> chosen by several other NFS implementations to deal with this problem. > > > > If locale-mismatch is a problem, it isn't clear to me that "mount.nfs" > > is the place to solve it. > > > > The problem is presented as: > > > > 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 '?'). > > > > So it sounds like the problem is copy/paste. I doubt that NFS addresses > > are the only things that can get corrupted. > > Maybe a more generic tool would be appropriate. > > I agree. The cited copy/paste use case is pretty weak. What a bold statement. Classic English-only user. Have you ever worked in a mixed language environment? Used VMware with Japanese Windows, and Japanese MAC OSX, and used clipboard between all these virtual machines? For example if you use MS File Explorer, and use "Copy Address", not "Copy Address As Text"? You'll find Unicode zero width space markers (class ZWSP), characters which are not displayed, to mark the begin and end of a Win32 object file name. Just Linux clipboard doesn't know about that little detail, worse, in UTF8 locales the characters are still invisible, because they use zero terminal cells (wcwidth() returns 0 for ZWSP characters!) you are literally screwed over. So copy in Windows, paste in Linux, paths will not work, unless you paste in the Linux terminal, then select© in the terminal the SAME path, and then paste it again. That works because it eliminates the ZWSP. BIDI (Bidirectional Text Layout) also uses such markers, depending on application. Shitty .NET apps for example. Finally, there is CTL, Complex Text Layout, for languages like Hindi (just 600 million people speak that as first or second language, so no need to care about them). Wanna hear what inter-OS clipboard usage does to such paths?
On 12/9/24 10:15 AM, Takeshi Nishimura wrote: > On Sun, Dec 8, 2024 at 4:58 PM Chuck Lever <chuck.lever@oracle.com> wrote: >> >> On 12/7/24 10:29 PM, NeilBrown wrote: >>> On Sun, 08 Dec 2024, Chuck Lever wrote: >>>> On 12/7/24 3:53 PM, NeilBrown wrote: >>>>> On Sat, 07 Dec 2024, Chuck Lever 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). >>>>> >>>>> Actually it isn't. You have already discussed the pub/root filehandle >>>>> difference. >>>> >>>> RFC 2224 specifies both. Pub vs. root filehandles are discussed >>>> there, along with how standard NFS URLs describe either situation. >>>> >>>> >>>>> The RFC doesn't know about v4. The RFC explicitly isn't a >>>>> standard. >>>> >>>> RFC 7532 contains the NFSv4 bits. RFC 2224 is not a Normative >>>> standard, like all early NFS-related RFCs, but it is a >>>> specification that other implementations cleave to. RFC 7532 >>>> /is/ a Normative standard. >>> >>> The usage in RFC 7532 is certainly more convincing than 2224. >>> >>>> >>>> >>>>> So I wonder if this is the right approach to solve the need. >>>>> >>>>> What is the needed? >>>>> Part of it seems to be non-ascii host names. Shouldn't we fix that for >>>>> the existing syntax? What are the barriers? >>>> >>>> Both non-ASCII hostnames (iDNA) and export paths can contain >>>> components with non-ASCII characters. >>> >>> But they cannot contain non-Unicode characters, so UTF-8 should be >>> sufficient. >>> >>>> >>>> The issue is how to specify certain code points when the client's >>>> locale might not support them. Using a URL seems to be the mechanism >>>> chosen by several other NFS implementations to deal with this problem. >>> >>> If locale-mismatch is a problem, it isn't clear to me that "mount.nfs" >>> is the place to solve it. >>> >>> The problem is presented as: >>> >>> 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 '?'). >>> >>> So it sounds like the problem is copy/paste. I doubt that NFS addresses >>> are the only things that can get corrupted. >>> Maybe a more generic tool would be appropriate. >> >> I agree. The cited copy/paste use case is pretty weak. > > What a bold statement. Classic English-only user. Dude, settle yourself. That comment is out of line and you are reading something into my remark that is not there. Let's stick with technical comments. The central problem, as you've laid out below, is that copy-paste isn't working for you. If copy-paste is a problem for NFS, it is a problem elsewhere too, thus it should be addressed where it can do the most good. Neil and I are both pointing out that there might be a better place to address this issue; we are not saying it's not consequential. In order to make a clear rationale for NFS URLs based on copy-paste, the below details are important to include. But we also need to understand why NFS (which has no notion of command line copy-paste) needs to tackle this problem. > Have you ever worked in a mixed language environment? Used VMware with > Japanese Windows, and Japanese MAC OSX, and used clipboard between all > these virtual machines? > For example if you use MS File Explorer, and use "Copy Address", not > "Copy Address As Text"? You'll find Unicode zero width space markers > (class ZWSP), characters which are not displayed, to mark the begin > and end of a Win32 object file name. > Just Linux clipboard doesn't know about that little detail, worse, in > UTF8 locales the characters are still invisible, because they use zero > terminal cells (wcwidth() returns 0 for ZWSP characters!) you are > literally screwed over. > So copy in Windows, paste in Linux, paths will not work, unless you > paste in the Linux terminal, then select© in the terminal the SAME > path, and then paste it again. That works because it eliminates the > ZWSP. Our point is: why not fix copy-paste, then? Please provide rationale for why this issue should be addressed by piecemeal changes to every system utility rather than addressing the copy-paste mechanism. If the issue has already been addressed in many other system utilities in this way, then please include examples in the patch description. > BIDI (Bidirectional Text Layout) also uses such markers, depending on > application. Shitty .NET apps for example. > > Finally, there is CTL, Complex Text Layout, for languages like Hindi > (just 600 million people speak that as first or second language, so no > need to care about them). > Wanna hear what inter-OS clipboard usage does to such paths?
On 8 Dec 2024, at 2:41, Cedric Blancher wrote: >>> - 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? > > NFSv3 is obsolete. Redhat support keeps telling us that for almost ten > years now. Red Hat does not consider NFSv3 to be obsolete. We've no plans to change our current support for it. Ben
On 12/10/24 6:46 AM, Benjamin Coddington wrote: > On 8 Dec 2024, at 2:41, Cedric Blancher wrote: >>>> - 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? >> >> NFSv3 is obsolete. Redhat support keeps telling us that for almost ten >> years now. > > Red Hat does not consider NFSv3 to be obsolete. We've no plans to change > our current support for it. Exactly! Although the one time I did suggest dropping V3 at one of the Bakeathons talks... It went over like a lead balloon :-) So V3 is here to stay! steved.
On Sun, 8 Dec 2024 at 23:12, NeilBrown <neilb@suse.de> wrote: > > + > > + /* > > + * |pnu.uctx->path| is in UTF-8, but we need the data > > + * in the current local locale's encoding, as mount(2) > > + * does not have something like a |MS_UTF8_SPEC| flag > > + * to indicate that the input path is in UTF-8, > > + * independently of the current locale > > + */ > > I don't understand this comment at all. > mount(2) doesn't care about locale (as far as I know). The "source" is > simply a string of bytes that it is up to the filesystem to interpret. > NFS will always interpret it as utf8. So no conversion is needed. Not all versions of NFS use UTF-8. For example if you have NFSv3 and the server runs ISO8859-16 (French), then the filenames are encoded using ISO8859-16, and the NFS client is assumed to use ISO8859-16 too. mount(2) has options to do filename encoding conversion NFSv4 is an improvement compared to NFSv3 because it uses UTF-8 on the wire, but if you use the (ANSSI/Clip-OS) nls=/iocharset= mount option you can enable filename encoding conversion there. Traditionally it is assumed that the admin runs its shell in the locale (implies filesystem encoding) used to do the mount(2), hence the iconv() to make sure there is no encoding mismatch. Finally: Not everyone and everything will use English (we frog eaters are proud of our language!) or UTF8, and it will remain that way for the foreseeable future. Ced
On 12/10/24 7:13 AM, Cedric Blancher wrote: > On Sun, 8 Dec 2024 at 23:12, NeilBrown <neilb@suse.de> wrote: >>> + >>> + /* >>> + * |pnu.uctx->path| is in UTF-8, but we need the data >>> + * in the current local locale's encoding, as mount(2) >>> + * does not have something like a |MS_UTF8_SPEC| flag >>> + * to indicate that the input path is in UTF-8, >>> + * independently of the current locale >>> + */ >> >> I don't understand this comment at all. >> mount(2) doesn't care about locale (as far as I know). The "source" is >> simply a string of bytes that it is up to the filesystem to interpret. >> NFS will always interpret it as utf8. So no conversion is needed. > > Not all versions of NFS use UTF-8. For example if you have NFSv3 and > the server runs ISO8859-16 (French), then the filenames are encoded > using ISO8859-16, and the NFS client is assumed to use ISO8859-16 too. I'm still trying to understand the usefulness of supporting NFSv3 as well. I understand that your usage requirement is only NFSv4. However, does your comment mean that there might be some cases where a client administrator might want to mount with NFSv3 using non-ASCII characters (of any flavor) in the export path? I'm also wondering about non-ASCII characters in the server hostname. That does not depend on which NFS version is in play. > mount(2) has options to do filename encoding conversion > NFSv4 is an improvement compared to NFSv3 because it uses UTF-8 on the > wire, but if you use the (ANSSI/Clip-OS) nls=/iocharset= mount option > you can enable filename encoding conversion there. > > Traditionally it is assumed that the admin runs its shell in the > locale (implies filesystem encoding) used to do the mount(2), hence > the iconv() to make sure there is no encoding mismatch. > > Finally: Not everyone and everything will use English (we frog eaters > are proud of our language!) or UTF8, and it will remain that way for > the foreseeable future. I'll repeat: none of us are denigrating languages or usages here. We're attempting to clarify the problem statement and refine the use cases.
On Tue, 10 Dec 2024, Cedric Blancher wrote: > On Sun, 8 Dec 2024 at 23:12, NeilBrown <neilb@suse.de> wrote: > > > + > > > + /* > > > + * |pnu.uctx->path| is in UTF-8, but we need the data > > > + * in the current local locale's encoding, as mount(2) > > > + * does not have something like a |MS_UTF8_SPEC| flag > > > + * to indicate that the input path is in UTF-8, > > > + * independently of the current locale > > > + */ > > > > I don't understand this comment at all. > > mount(2) doesn't care about locale (as far as I know). The "source" is > > simply a string of bytes that it is up to the filesystem to interpret. > > NFS will always interpret it as utf8. So no conversion is needed. > > Not all versions of NFS use UTF-8. For example if you have NFSv3 and > the server runs ISO8859-16 (French), then the filenames are encoded > using ISO8859-16, and the NFS client is assumed to use ISO8859-16 too. > mount(2) has options to do filename encoding conversion > NFSv4 is an improvement compared to NFSv3 because it uses UTF-8 on the > wire, but if you use the (ANSSI/Clip-OS) nls=/iocharset= mount option > you can enable filename encoding conversion there. I cannot find any evidence that nfs supports iocharset- or nls= Other filesystems do: fat, isofs, jfs, smb ntfs3 and others. But not NFS. nfs-utils doesn't appear to have any support either. So I think that the string you pass on the mount command line, or in /etc/fstab, will be passed unchanged over-the-wire to mountd (For NFSv3) or passed to the kernel and thence to nfsd (for NFSv4). Do you have evidence to prove otherwise? i.e. can you demonstrate a case where changing the LOCALE causes a mount command that previously worked to stop working? Thanks, NeilBrown
On Wed, 11 Dec 2024 at 05:26, NeilBrown <neilb@suse.de> wrote: > > On Tue, 10 Dec 2024, Cedric Blancher wrote: > > On Sun, 8 Dec 2024 at 23:12, NeilBrown <neilb@suse.de> wrote: > > > > + > > > > + /* > > > > + * |pnu.uctx->path| is in UTF-8, but we need the data > > > > + * in the current local locale's encoding, as mount(2) > > > > + * does not have something like a |MS_UTF8_SPEC| flag > > > > + * to indicate that the input path is in UTF-8, > > > > + * independently of the current locale > > > > + */ > > > > > > I don't understand this comment at all. > > > mount(2) doesn't care about locale (as far as I know). The "source" is > > > simply a string of bytes that it is up to the filesystem to interpret. > > > NFS will always interpret it as utf8. So no conversion is needed. > > > > Not all versions of NFS use UTF-8. For example if you have NFSv3 and > > the server runs ISO8859-16 (French), then the filenames are encoded > > using ISO8859-16, and the NFS client is assumed to use ISO8859-16 too. > > mount(2) has options to do filename encoding conversion > > NFSv4 is an improvement compared to NFSv3 because it uses UTF-8 on the > > wire, but if you use the (ANSSI/Clip-OS) nls=/iocharset= mount option > > you can enable filename encoding conversion there. > > I cannot find any evidence that nfs supports iocharset- or nls= > Other filesystems do: fat, isofs, jfs, smb ntfs3 and others. > But not NFS. > nfs-utils doesn't appear to have any support either. Gentoo-based ClipOS&RED FLAG Linux certainly support that, first one for iso8859-16 mapping, and the second for GB18030. I don't know if this is in mainline or not. In either case mapping to local encoding is required, or as Roland's comment indicates a mount syscall parameter to indicate the encoding of the export path string. > > So I think that the string you pass on the mount command line, or in > /etc/fstab, will be passed unchanged over-the-wire to mountd (For NFSv3) > or passed to the kernel and thence to nfsd (for NFSv4). > > Do you have evidence to prove otherwise? i.e. can you demonstrate a > case where changing the LOCALE causes a mount command that previously > worked to stop working? For SMB certainly yes. For NFSv3 certainly yes too, for example if the NFS server runs on a filesystem with a different encoding than the client. Might be even a subset or superset of the encoding used by the other side, e.g. server uses Level GBK/5, but client only Level GBK/3. Ced
On Sat, 7 Dec 2024 at 16:21, Chuck Lever <chuck.lever@oracle.com> wrote: > >> 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. > > Here's the point of using a library: > > A popular library implementation has been very well reviewed and is > used by a number of applications. That gives a high degree of confidence > that there are fewer bugs (in particular, security-related bugs). The > library might be actively maintained, and so we don't have to do that > work. > > Something like liburiparser might be more than we need for this > particular job, but code re-use is pretty foundational. If liburiparser > isn't a good fit, look for something that is a better fit. I can have a stab at this unless Roland objects Ced
From e7b5a3ac981727e4585288bd66d1a9b2dea045dc Mon Sep 17 00:00:00 2001 From: Roland Mainz <roland.mainz@nrubsig.org> 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 Japanese, Chinese, French etc. characters) and ASCII-compatible descriptions of NFSv4 server resources (exports). Reviewed-by: Martin Wege <martin.l.wege@gmail.com> Signed-off-by: Cedric Blancher <cedric.blancher@gmail.com> --- utils/mount/Makefile.am | 3 +- utils/mount/nfs4mount.c | 69 +++++++- utils/mount/nfsmount.c | 93 ++++++++-- utils/mount/parse_dev.c | 67 ++++++-- utils/mount/stropts.c | 96 ++++++++++- utils/mount/urlparser1.c | 358 +++++++++++++++++++++++++++++++++++++++ utils/mount/urlparser1.h | 60 +++++++ utils/mount/utils.c | 155 +++++++++++++++++ utils/mount/utils.h | 23 +++ 9 files changed, 887 insertions(+), 37 deletions(-) create mode 100644 utils/mount/urlparser1.c create mode 100644 utils/mount/urlparser1.h diff --git a/utils/mount/Makefile.am b/utils/mount/Makefile.am index 83a8ee1c..0e4cab3e 100644 --- a/utils/mount/Makefile.am +++ b/utils/mount/Makefile.am @@ -13,7 +13,8 @@ sbin_PROGRAMS = mount.nfs EXTRA_DIST = nfsmount.conf $(man8_MANS) $(man5_MANS) mount_common = error.c network.c token.c \ parse_opt.c parse_dev.c \ - nfsmount.c nfs4mount.c stropts.c\ + nfsmount.c nfs4mount.c \ + urlparser1.c urlparser1.h stropts.c \ mount_constants.h error.h network.h token.h \ parse_opt.h parse_dev.h \ nfs4_mount.h stropts.h version.h \ diff --git a/utils/mount/nfs4mount.c b/utils/mount/nfs4mount.c index 0fe142a7..8e4fbf30 100644 --- a/utils/mount/nfs4mount.c +++ b/utils/mount/nfs4mount.c @@ -50,8 +50,10 @@ #include "mount_constants.h" #include "nfs4_mount.h" #include "nfs_mount.h" +#include "urlparser1.h" #include "error.h" #include "network.h" +#include "utils.h" #if defined(VAR_LOCK_DIR) #define DEFAULT_DIR VAR_LOCK_DIR @@ -182,7 +184,7 @@ int nfs4mount(const char *spec, const char *node, int flags, int num_flavour = 0; int ip_addr_in_opts = 0; - char *hostname, *dirname, *old_opts; + char *hostname, *dirname, *mb_dirname = NULL, *old_opts; char new_opts[1024]; char *opt, *opteq; char *s; @@ -192,15 +194,66 @@ int nfs4mount(const char *spec, const char *node, int flags, int retry; int retval = EX_FAIL; time_t timeout, t; + int nfs_port = NFS_PORT; + parsed_nfs_url pnu; + + (void)memset(&pnu, 0, sizeof(parsed_nfs_url)); if (strlen(spec) >= sizeof(hostdir)) { nfs_error(_("%s: excessively long host:dir argument\n"), progname); goto fail; } - strcpy(hostdir, spec); - if (parse_devname(hostdir, &hostname, &dirname)) - goto fail; + + /* + * Support nfs://-URLS per RFC 2224 ("NFS URL + * SCHEME", see https://www.rfc-editor.org/rfc/rfc2224.html), + * including custom port (nfs://hostname@port/path/...) + * and URL parameter (e.g. nfs://.../?param1=val1¶m2=val2 + * support + */ + if (is_spec_nfs_url(spec)) { + if (!mount_parse_nfs_url(spec, &pnu)) { + goto fail; + } + + /* + * |pnu.uctx->path| is in UTF-8, but we need the data + * in the current local locale's encoding, as mount(2) + * does not have something like a |MS_UTF8_SPEC| flag + * to indicate that the input path is in UTF-8, + * independently of the current locale + */ + hostname = pnu.uctx->hostport.hostname; + dirname = mb_dirname = utf8str2mbstr(pnu.uctx->path); + + (void)snprintf(hostdir, sizeof(hostdir), "%s:/%s", + hostname, dirname); + spec = hostdir; + + if (pnu.uctx->hostport.port != -1) { + nfs_port = pnu.uctx->hostport.port; + } + + /* + * Values added here based on URL parameters + * should be added the front of the list of options, + * so users can override the nfs://-URL given default. + * + * FIXME: We do not do that here for |MS_RDONLY|! + */ + if (pnu.mount_params.read_only != TRIS_BOOL_NOT_SET) { + if (pnu.mount_params.read_only) + flags |= MS_RDONLY; + else + flags &= ~MS_RDONLY; + } + } else { + (void)strcpy(hostdir, spec); + + if (parse_devname(hostdir, &hostname, &dirname)) + goto fail; + } if (fill_ipv4_sockaddr(hostname, &server_addr)) goto fail; @@ -247,7 +300,7 @@ int nfs4mount(const char *spec, const char *node, int flags, /* * NFSv4 specifies that the default port should be 2049 */ - server_addr.sin_port = htons(NFS_PORT); + server_addr.sin_port = htons(nfs_port); /* parse options */ @@ -474,8 +527,14 @@ int nfs4mount(const char *spec, const char *node, int flags, } } + mount_free_parse_nfs_url(&pnu); + free(mb_dirname); + return EX_SUCCESS; fail: + mount_free_parse_nfs_url(&pnu); + free(mb_dirname); + return retval; } diff --git a/utils/mount/nfsmount.c b/utils/mount/nfsmount.c index a1c92fe8..e61d718a 100644 --- a/utils/mount/nfsmount.c +++ b/utils/mount/nfsmount.c @@ -63,11 +63,13 @@ #include "xcommon.h" #include "mount.h" #include "nfs_mount.h" +#include "urlparser1.h" #include "mount_constants.h" #include "nls.h" #include "error.h" #include "network.h" #include "version.h" +#include "utils.h" #ifdef HAVE_RPCSVC_NFS_PROT_H #include <rpcsvc/nfs_prot.h> @@ -493,7 +495,7 @@ nfsmount(const char *spec, const char *node, int flags, char **extra_opts, int fake, int running_bg) { char hostdir[1024]; - char *hostname, *dirname, *old_opts, *mounthost = NULL; + char *hostname, *dirname, *mb_dirname = NULL, *old_opts, *mounthost = NULL; char new_opts[1024], cbuf[1024]; static struct nfs_mount_data data; int val; @@ -521,29 +523,79 @@ nfsmount(const char *spec, const char *node, int flags, time_t t; time_t prevt; time_t timeout; + int nfsurl_port = -1; + parsed_nfs_url pnu; + + (void)memset(&pnu, 0, sizeof(parsed_nfs_url)); if (strlen(spec) >= sizeof(hostdir)) { nfs_error(_("%s: excessively long host:dir argument"), progname); goto fail; } - strcpy(hostdir, spec); - if ((s = strchr(hostdir, ':'))) { - hostname = hostdir; - dirname = s + 1; - *s = '\0'; - /* Ignore all but first hostname in replicated mounts - until they can be fully supported. (mack@sgi.com) */ - if ((s = strchr(hostdir, ','))) { + + /* + * Support nfs://-URLS per RFC 2224 ("NFS URL + * SCHEME", see https://www.rfc-editor.org/rfc/rfc2224.html), + * including custom port (nfs://hostname@port/path/...) + * and URL parameter (e.g. nfs://.../?param1=val1¶m2=val2 + * support + */ + if (is_spec_nfs_url(spec)) { + if (!mount_parse_nfs_url(spec, &pnu)) { + goto fail; + } + + /* + * |pnu.uctx->path| is in UTF-8, but we need the data + * in the current local locale's encoding, as mount(2) + * does not have something like a |MS_UTF8_SPEC| flag + * to indicate that the input path is in UTF-8, + * independently of the current locale + */ + hostname = pnu.uctx->hostport.hostname; + dirname = mb_dirname = utf8str2mbstr(pnu.uctx->path); + + (void)snprintf(hostdir, sizeof(hostdir), "%s:/%s", + hostname, dirname); + spec = hostdir; + + if (pnu.uctx->hostport.port != -1) { + nfsurl_port = pnu.uctx->hostport.port; + } + + /* + * Values added here based on URL parameters + * should be added the front of the list of options, + * so users can override the nfs://-URL given default. + * + * FIXME: We do not do that here for |MS_RDONLY|! + */ + if (pnu.mount_params.read_only != TRIS_BOOL_NOT_SET) { + if (pnu.mount_params.read_only) + flags |= MS_RDONLY; + else + flags &= ~MS_RDONLY; + } + } else { + (void)strcpy(hostdir, spec); + if ((s = strchr(hostdir, ':'))) { + hostname = hostdir; + dirname = s + 1; *s = '\0'; - nfs_error(_("%s: warning: " - "multiple hostnames not supported"), + /* Ignore all but first hostname in replicated mounts + until they can be fully supported. (mack@sgi.com) */ + if ((s = strchr(hostdir, ','))) { + *s = '\0'; + nfs_error(_("%s: warning: " + "multiple hostnames not supported"), progname); - } - } else { - nfs_error(_("%s: directory to mount not in host:dir format"), + } + } else { + nfs_error(_("%s: directory to mount not in host:dir format"), progname); - goto fail; + goto fail; + } } if (!nfs_gethostbyname(hostname, nfs_saddr)) @@ -579,6 +631,14 @@ nfsmount(const char *spec, const char *node, int flags, memset(nfs_pmap, 0, sizeof(*nfs_pmap)); nfs_pmap->pm_prog = NFS_PROGRAM; + if (nfsurl_port != -1) { + /* + * Set custom TCP port defined by a nfs://-URL here, + * so $ mount -o port ... # can be used to override + */ + nfs_pmap->pm_port = nfsurl_port; + } + /* parse options */ new_opts[0] = 0; if (!parse_options(old_opts, &data, &bg, &retry, &mnt_server, &nfs_server, @@ -863,10 +923,13 @@ noauth_flavors: } } + mount_free_parse_nfs_url(&pnu); + return EX_SUCCESS; /* abort */ fail: + mount_free_parse_nfs_url(&pnu); if (fsock != -1) close(fsock); return retval; diff --git a/utils/mount/parse_dev.c b/utils/mount/parse_dev.c index 2ade5d5d..d9f8cf59 100644 --- a/utils/mount/parse_dev.c +++ b/utils/mount/parse_dev.c @@ -27,6 +27,8 @@ #include "xcommon.h" #include "nls.h" #include "parse_dev.h" +#include "urlparser1.h" +#include "utils.h" #ifndef NFS_MAXHOSTNAME #define NFS_MAXHOSTNAME (255) @@ -179,17 +181,62 @@ static int nfs_parse_square_bracket(const char *dev, } /* - * RFC 2224 says an NFS client must grok "public file handles" to - * support NFS URLs. Linux doesn't do that yet. Print a somewhat - * helpful error message in this case instead of pressing forward - * with the mount request and failing with a cryptic error message - * later. + * Support nfs://-URLS per RFC 2224 ("NFS URL + * SCHEME", see https://www.rfc-editor.org/rfc/rfc2224.html), + * including port support (nfs://hostname@port/path/...) */ -static int nfs_parse_nfs_url(__attribute__((unused)) const char *dev, - __attribute__((unused)) char **hostname, - __attribute__((unused)) char **pathname) +static int nfs_parse_nfs_url(const char *dev, + char **out_hostname, + char **out_pathname) { - nfs_error(_("%s: NFS URLs are not supported"), progname); + parsed_nfs_url pnu; + + if (out_hostname) + *out_hostname = NULL; + if (out_pathname) + *out_pathname = NULL; + + /* + * Support nfs://-URLS per RFC 2224 ("NFS URL + * SCHEME", see https://www.rfc-editor.org/rfc/rfc2224.html), + * including custom port (nfs://hostname@port/path/...) + * and URL parameter (e.g. nfs://.../?param1=val1¶m2=val2 + * support + */ + if (!mount_parse_nfs_url(dev, &pnu)) { + goto fail; + } + + if (pnu.uctx->hostport.port != -1) { + /* NOP here, unless we switch from hostname to hostport */ + } + + if (out_hostname) + *out_hostname = strdup(pnu.uctx->hostport.hostname); + if (out_pathname) + *out_pathname = utf8str2mbstr(pnu.uctx->path); + + if (((out_hostname)?(*out_hostname == NULL):0) || + ((out_pathname)?(*out_pathname == NULL):0)) { + nfs_error(_("%s: out of memory"), + progname); + goto fail; + } + + mount_free_parse_nfs_url(&pnu); + + return 1; + +fail: + mount_free_parse_nfs_url(&pnu); + if (out_hostname) { + free(*out_hostname); + *out_hostname = NULL; + } + if (out_pathname) { + free(*out_pathname); + *out_pathname = NULL; + } return 0; } @@ -223,7 +270,7 @@ int nfs_parse_devname(const char *devname, return nfs_pdn_nomem_err(); if (*dev == '[') result = nfs_parse_square_bracket(dev, hostname, pathname); - else if (strncmp(dev, "nfs://", 6) == 0) + else if (is_spec_nfs_url(dev)) result = nfs_parse_nfs_url(dev, hostname, pathname); else result = nfs_parse_simple_hostname(dev, hostname, pathname); diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c index 23f0a8c0..ad92ab78 100644 --- a/utils/mount/stropts.c +++ b/utils/mount/stropts.c @@ -42,6 +42,7 @@ #include "nls.h" #include "nfsrpc.h" #include "mount_constants.h" +#include "urlparser1.h" #include "stropts.h" #include "error.h" #include "network.h" @@ -50,6 +51,7 @@ #include "parse_dev.h" #include "conffile.h" #include "misc.h" +#include "utils.h" #ifndef NFS_PROGRAM #define NFS_PROGRAM (100003) @@ -643,24 +645,106 @@ static int nfs_sys_mount(struct nfsmount_info *mi, struct mount_options *opts) { char *options = NULL; int result; + int nfs_port = 2049; if (mi->fake) return 1; - if (po_join(opts, &options) == PO_FAILED) { - errno = EIO; - return 0; - } + /* + * Support nfs://-URLS per RFC 2224 ("NFS URL + * SCHEME", see https://www.rfc-editor.org/rfc/rfc2224.html), + * including custom port (nfs://hostname@port/path/...) + * and URL parameter (e.g. nfs://.../?param1=val1¶m2=val2 + * support + */ + if (is_spec_nfs_url(mi->spec)) { + parsed_nfs_url pnu; + char *mb_path; + char mount_source[1024]; + + if (!mount_parse_nfs_url(mi->spec, &pnu)) { + result = 1; + errno = EINVAL; + goto done; + } + + /* + * |pnu.uctx->path| is in UTF-8, but we need the data + * in the current local locale's encoding, as mount(2) + * does not have something like a |MS_UTF8_SPEC| flag + * to indicate that the input path is in UTF-8, + * independently of the current locale + */ + mb_path = utf8str2mbstr(pnu.uctx->path); + if (!mb_path) { + nfs_error(_("%s: Could not convert path to local encoding."), + progname); + mount_free_parse_nfs_url(&pnu); + result = 1; + errno = EINVAL; + goto done; + } + + (void)snprintf(mount_source, sizeof(mount_source), + "%s:/%s", + pnu.uctx->hostport.hostname, + mb_path); + free(mb_path); + + if (pnu.uctx->hostport.port != -1) { + nfs_port = pnu.uctx->hostport.port; + } - result = mount(mi->spec, mi->node, mi->type, + /* + * Insert "port=" option with the value from the nfs:// + * URL at the beginning of the list of options, so + * users can override it with $ mount.nfs4 -o port= #, + * e.g. + * $ mount.nfs4 -o port=1234 nfs://10.49.202.230:400//bigdisk /mnt4 # + * should use port 1234, and not port 400 as specified + * in the URL. + */ + char portoptbuf[5+32+1]; + (void)snprintf(portoptbuf, sizeof(portoptbuf), "port=%d", nfs_port); + (void)po_insert(opts, portoptbuf); + + if (pnu.mount_params.read_only != TRIS_BOOL_NOT_SET) { + if (pnu.mount_params.read_only) + mi->flags |= MS_RDONLY; + else + mi->flags &= ~MS_RDONLY; + } + + mount_free_parse_nfs_url(&pnu); + + if (po_join(opts, &options) == PO_FAILED) { + errno = EIO; + result = 1; + goto done; + } + + result = mount(mount_source, mi->node, mi->type, + mi->flags & ~(MS_USER|MS_USERS), options); + free(options); + } else { + if (po_join(opts, &options) == PO_FAILED) { + errno = EIO; + result = 1; + goto done; + } + + result = mount(mi->spec, mi->node, mi->type, mi->flags & ~(MS_USER|MS_USERS), options); - free(options); + free(options); + } if (verbose && result) { int save = errno; nfs_error(_("%s: mount(2): %s"), progname, strerror(save)); errno = save; } + +done: return !result; } diff --git a/utils/mount/urlparser1.c b/utils/mount/urlparser1.c new file mode 100644 index 00000000..d4c6f339 --- /dev/null +++ b/utils/mount/urlparser1.c @@ -0,0 +1,358 @@ +/* + * MIT License + * + * Copyright (c) 2024 Roland Mainz <roland.mainz@nrubsig.org> + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +/* urlparser1.c - simple URL parser */ + +#include <stdlib.h> +#include <stdbool.h> +#include <string.h> +#include <ctype.h> +#include <stdio.h> + +#ifdef DBG_USE_WIDECHAR +#include <wchar.h> +#include <locale.h> +#include <io.h> +#include <fcntl.h> +#endif /* DBG_USE_WIDECHAR */ + +#include "urlparser1.h" + +typedef struct _url_parser_context_private { + url_parser_context c; + + /* Private data */ + char *parameter_string_buff; +} url_parser_context_private; + +#define MAX_URL_PARAMETERS 256 + +/* + * Original extended regular expression: + * + * "^" + * "(.+?)" // scheme + * "://" // '://' + * "(" // login + * "(?:" + * "(.+?)" // user (optional) + * "(?::(.+))?" // password (optional) + * "@" + * ")?" + * "(" // hostport + * "(.+?)" // host + * "(?::([[:digit:]]+))?" // port (optional) + * ")" + * ")" + * "(?:/(.*?))?" // path (optional) + * "(?:\?(.*?))?" // URL parameters (optional) + * "$" + */ + +#define DBGNULLSTR(s) (((s)!=NULL)?(s):"<NULL>") +#if 0 || defined(TEST_URLPARSER) +#define D(x) x +#else +#define D(x) +#endif + +#ifdef DBG_USE_WIDECHAR +/* + * Use wide-char APIs on WIN32, otherwise we cannot output + * Japanese/Chinese/etc correctly + */ +#define DBG_PUTS(str, fp) fputws(L"" str, (fp)) +#define DBG_PUTC(c, fp) fputwc(btowc(c), (fp)) +#define DBG_PRINTF(fp, fmt, ...) fwprintf((fp), L"" fmt, __VA_ARGS__) +#else +#define DBG_PUTS(str, fp) fputs((str), (fp)) +#define DBG_PUTC(c, fp) fputc((c), (fp)) +#define DBG_PRINTF(fp, fmt, ...) fprintf((fp), fmt, __VA_ARGS__) +#endif /* DBG_USE_WIDECHAR */ + +static +void urldecodestr(char *outbuff, const char *buffer, size_t len) +{ + size_t i, j; + + for (i = j = 0 ; i < len ; ) { + switch (buffer[i]) { + case '%': + if ((i + 2) < len) { + if (isxdigit((int)buffer[i+1]) && isxdigit((int)buffer[i+2])) { + const char hexstr[3] = { + buffer[i+1], + buffer[i+2], + '\0' + }; + outbuff[j++] = (unsigned char)strtol(hexstr, NULL, 16); + i += 3; + } else { + /* invalid hex digit */ + outbuff[j++] = buffer[i]; + i++; + } + } else { + /* incomplete hex digit */ + outbuff[j++] = buffer[i]; + i++; + } + break; + case '+': + outbuff[j++] = ' '; + i++; + break; + default: + outbuff[j++] = buffer[i++]; + break; + } + } + + outbuff[j] = '\0'; +} + +url_parser_context *url_parser_create_context(const char *in_url, unsigned int flags) +{ + url_parser_context_private *uctx; + char *s; + size_t in_url_len; + size_t context_len; + + /* |flags| is for future extensions */ + (void)flags; + + if (!in_url) + return NULL; + + in_url_len = strlen(in_url); + + context_len = sizeof(url_parser_context_private) + + ((in_url_len+1)*6) + + (sizeof(url_parser_name_value)*MAX_URL_PARAMETERS)+sizeof(void*); + uctx = malloc(context_len); + if (!uctx) + return NULL; + + s = (void *)(uctx+1); + uctx->c.in_url = s; s+= in_url_len+1; + (void)strcpy(uctx->c.in_url, in_url); + uctx->c.scheme = s; s+= in_url_len+1; + uctx->c.login.username = s; s+= in_url_len+1; + uctx->c.hostport.hostname = s; s+= in_url_len+1; + uctx->c.path = s; s+= in_url_len+1; + uctx->c.hostport.port = -1; + uctx->c.num_parameters = -1; + uctx->c.parameters = (void *)s; s+= (sizeof(url_parser_name_value)*MAX_URL_PARAMETERS)+sizeof(void*); + uctx->parameter_string_buff = s; s+= in_url_len+1; + + return &uctx->c; +} + +int url_parser_parse(url_parser_context *ctx) +{ + url_parser_context_private *uctx = (url_parser_context_private *)ctx; + + D((void)DBG_PRINTF(stderr, "## parser in_url='%s'\n", uctx->c.in_url)); + + char *s; + const char *urlstr = uctx->c.in_url; + size_t slen; + + s = strstr(urlstr, "://"); + if (!s) { + D((void)DBG_PUTS("url_parser: Not an URL\n", stderr)); + return -1; + } + + slen = s-urlstr; + (void)memcpy(uctx->c.scheme, urlstr, slen); + uctx->c.scheme[slen] = '\0'; + urlstr += slen + 3; + + D((void)DBG_PRINTF(stdout, "scheme='%s', rest='%s'\n", uctx->c.scheme, urlstr)); + + s = strstr(urlstr, "@"); + if (s) { + /* URL has user/password */ + slen = s-urlstr; + urldecodestr(uctx->c.login.username, urlstr, slen); + urlstr += slen + 1; + + s = strstr(uctx->c.login.username, ":"); + if (s) { + /* found passwd */ + uctx->c.login.passwd = s+1; + *s = '\0'; + } + else { + uctx->c.login.passwd = NULL; + } + + /* catch password-only URLs */ + if (uctx->c.login.username[0] == '\0') + uctx->c.login.username = NULL; + } + else { + uctx->c.login.username = NULL; + uctx->c.login.passwd = NULL; + } + + D((void)DBG_PRINTF(stdout, "login='%s', passwd='%s', rest='%s'\n", + DBGNULLSTR(uctx->c.login.username), + DBGNULLSTR(uctx->c.login.passwd), + DBGNULLSTR(urlstr))); + + char *raw_parameters; + + uctx->c.num_parameters = 0; + raw_parameters = strstr(urlstr, "?"); + /* Do we have a non-empty parameter string ? */ + if (raw_parameters && (raw_parameters[1] != '\0')) { + *raw_parameters++ = '\0'; + D((void)DBG_PRINTF(stdout, "raw parameters = '%s'\n", raw_parameters)); + + char *ps = raw_parameters; + char *pv; /* parameter value */ + char *na; /* next '&' */ + char *pb = uctx->parameter_string_buff; + char *pname; + char *pvalue; + ssize_t pi; + + for (pi = 0; pi < MAX_URL_PARAMETERS ; pi++) { + pname = ps; + + /* + * Handle parameters without value, + * e.g. "path?name1&name2=value2" + */ + na = strstr(ps, "&"); + pv = strstr(ps, "="); + if (pv && (na?(na > pv):true)) { + *pv++ = '\0'; + pvalue = pv; + ps = pv; + } + else { + pvalue = NULL; + } + + if (na) { + *na++ = '\0'; + } + + /* URLDecode parameter name */ + urldecodestr(pb, pname, strlen(pname)); + uctx->c.parameters[pi].name = pb; + pb += strlen(uctx->c.parameters[pi].name)+1; + + /* URLDecode parameter value */ + if (pvalue) { + urldecodestr(pb, pvalue, strlen(pvalue)); + uctx->c.parameters[pi].value = pb; + pb += strlen(uctx->c.parameters[pi].value)+1; + } + else { + uctx->c.parameters[pi].value = NULL; + } + + /* Next '&' ? */ + if (!na) + break; + + ps = na; + } + + uctx->c.num_parameters = pi+1; + } + + s = strstr(urlstr, "/"); + if (s) { + /* URL has hostport */ + slen = s-urlstr; + urldecodestr(uctx->c.hostport.hostname, urlstr, slen); + urlstr += slen + 1; + + /* + * check for addresses within '[' and ']', like + * IPv6 addresses + */ + s = uctx->c.hostport.hostname; + if (s[0] == '[') + s = strstr(s, "]"); + + if (s == NULL) { + D((void)DBG_PUTS("url_parser: Unmatched '[' in hostname\n", stderr)); + return -1; + } + + s = strstr(s, ":"); + if (s) { + /* found port number */ + uctx->c.hostport.port = atoi(s+1); + *s = '\0'; + } + } + else { + (void)strcpy(uctx->c.hostport.hostname, urlstr); + uctx->c.path = NULL; + urlstr = NULL; + } + + D( + (void)DBG_PRINTF(stdout, + "hostport='%s', port=%d, rest='%s', num_parameters=%d\n", + DBGNULLSTR(uctx->c.hostport.hostname), + uctx->c.hostport.port, + DBGNULLSTR(urlstr), + (int)uctx->c.num_parameters); + ); + + + D( + ssize_t dpi; + for (dpi = 0 ; dpi < uctx->c.num_parameters ; dpi++) { + (void)DBG_PRINTF(stdout, + "param[%d]: name='%s'/value='%s'\n", + (int)dpi, + uctx->c.parameters[dpi].name, + DBGNULLSTR(uctx->c.parameters[dpi].value)); + } + ); + + if (!urlstr) { + goto done; + } + + urldecodestr(uctx->c.path, urlstr, strlen(urlstr)); + D((void)DBG_PRINTF(stdout, "path='%s'\n", uctx->c.path)); + +done: + return 0; +} + +void url_parser_free_context(url_parser_context *c) +{ + free(c); +} diff --git a/utils/mount/urlparser1.h b/utils/mount/urlparser1.h new file mode 100644 index 00000000..515eea9d --- /dev/null +++ b/utils/mount/urlparser1.h @@ -0,0 +1,60 @@ +/* + * MIT License + * + * Copyright (c) 2024 Roland Mainz <roland.mainz@nrubsig.org> + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +/* urlparser1.h - header for simple URL parser */ + +#ifndef __URLPARSER1_H__ +#define __URLPARSER1_H__ 1 + +#include <stdlib.h> + +typedef struct _url_parser_name_value { + char *name; + char *value; +} url_parser_name_value; + +typedef struct _url_parser_context { + char *in_url; + + char *scheme; + struct { + char *username; + char *passwd; + } login; + struct { + char *hostname; + signed int port; + } hostport; + char *path; + + ssize_t num_parameters; + url_parser_name_value *parameters; +} url_parser_context; + +/* Prototypes */ +url_parser_context *url_parser_create_context(const char *in_url, unsigned int flags); +int url_parser_parse(url_parser_context *uctx); +void url_parser_free_context(url_parser_context *c); + +#endif /* !__URLPARSER1_H__ */ diff --git a/utils/mount/utils.c b/utils/mount/utils.c index b7562a47..2d4cfa5a 100644 --- a/utils/mount/utils.c +++ b/utils/mount/utils.c @@ -28,6 +28,7 @@ #include <unistd.h> #include <sys/types.h> #include <sys/stat.h> +#include <iconv.h> #include "sockaddr.h" #include "nfs_mount.h" @@ -173,3 +174,157 @@ int nfs_umount23(const char *devname, char *string) free(dirname); return result; } + +/* Convert UTF-8 string to multibyte string in the current locale */ +char *utf8str2mbstr(const char *utf8_str) +{ + iconv_t cd; + + cd = iconv_open("UTF-8", ""); + if (cd == (iconv_t)-1) { + perror("utf8str2mbstr: iconv_open failed"); + return NULL; + } + + size_t inbytesleft = strlen(utf8_str); + char *inbuf = (char *)utf8_str; + size_t outbytesleft = inbytesleft*4+1; + char *outbuf = malloc(outbytesleft); + char *outbuf_orig = outbuf; + + if (!outbuf) { + perror("utf8str2mbstr: out of memory"); + (void)iconv_close(cd); + return NULL; + } + + int ret = iconv(cd, &inbuf, &inbytesleft, &outbuf, &outbytesleft); + if (ret == -1) { + perror("utf8str2mbstr: iconv() failed"); + free(outbuf_orig); + (void)iconv_close(cd); + return NULL; + } + + *outbuf = '\0'; + + (void)iconv_close(cd); + return outbuf_orig; +} + +/* fixme: We should use |bool|! */ +int is_spec_nfs_url(const char *spec) +{ + return (!strncmp(spec, "nfs://", 6)); +} + +int mount_parse_nfs_url(const char *spec, parsed_nfs_url *pnu) +{ + int result = 1; + url_parser_context *uctx = NULL; + + (void)memset(pnu, 0, sizeof(parsed_nfs_url)); + pnu->mount_params.read_only = TRIS_BOOL_NOT_SET; + + uctx = url_parser_create_context(spec, 0); + if (!uctx) { + nfs_error(_("%s: out of memory"), + progname); + result = 1; + goto done; + } + + if (url_parser_parse(uctx) < 0) { + nfs_error(_("%s: Error parsing nfs://-URL."), + progname); + result = 1; + goto done; + } + if (uctx->login.username || uctx->login.passwd) { + nfs_error(_("%s: Username/Password are not defined for nfs://-URL."), + progname); + result = 1; + goto done; + } + if (!uctx->path) { + nfs_error(_("%s: Path missing in nfs://-URL."), + progname); + result = 1; + goto done; + } + if (uctx->path[0] != '/') { + nfs_error(_("%s: Relative nfs://-URLs are not supported."), + progname); + result = 1; + goto done; + } + + if (uctx->num_parameters > 0) { + int pi; + const char *pname; + const char *pvalue; + + /* + * Values added here based on URL parameters + * should be added the front of the list of options, + * so users can override the nfs://-URL given default. + */ + for (pi = 0; pi < uctx->num_parameters ; pi++) { + pname = uctx->parameters[pi].name; + pvalue = uctx->parameters[pi].value; + + if (!strcmp(pname, "rw")) { + if ((pvalue == NULL) || (!strcmp(pvalue, "1"))) { + pnu->mount_params.read_only = TRIS_BOOL_FALSE; + } + else if (!strcmp(pvalue, "0")) { + pnu->mount_params.read_only = TRIS_BOOL_TRUE; + } + else { + nfs_error(_("%s: Unsupported nfs://-URL " + "parameter '%s' value '%s'."), + progname, pname, pvalue); + result = 1; + goto done; + } + } + else if (!strcmp(pname, "ro")) { + if ((pvalue == NULL) || (!strcmp(pvalue, "1"))) { + pnu->mount_params.read_only = TRIS_BOOL_TRUE; + } + else if (!strcmp(pvalue, "0")) { + pnu->mount_params.read_only = TRIS_BOOL_FALSE; + } + else { + nfs_error(_("%s: Unsupported nfs://-URL " + "parameter '%s' value '%s'."), + progname, pname, pvalue); + result = 1; + goto done; + } + } + else { + nfs_error(_("%s: Unsupported nfs://-URL " + "parameter '%s'."), + progname, pname); + result = 1; + goto done; + } + } + } + + result = 0; +done: + if (result != 0) { + url_parser_free_context(uctx); + return 0; + } + + pnu->uctx = uctx; + return 1; +} + +void mount_free_parse_nfs_url(parsed_nfs_url *pnu) +{ + url_parser_free_context(pnu->uctx); +} diff --git a/utils/mount/utils.h b/utils/mount/utils.h index 224918ae..465c0a5e 100644 --- a/utils/mount/utils.h +++ b/utils/mount/utils.h @@ -24,13 +24,36 @@ #define _NFS_UTILS_MOUNT_UTILS_H #include "parse_opt.h" +#include "urlparser1.h" +/* Boolean with three states: { not_set, false, true */ +typedef signed char tristate_bool; +#define TRIS_BOOL_NOT_SET (-1) +#define TRIS_BOOL_TRUE (1) +#define TRIS_BOOL_FALSE (0) + +#define TRIS_BOOL_GET_VAL(tsb, tsb_default) \ + (((tsb)!=TRIS_BOOL_NOT_SET)?(tsb):(tsb_default)) + +typedef struct _parsed_nfs_url { + url_parser_context *uctx; + struct { + tristate_bool read_only; + } mount_params; +} parsed_nfs_url; + +/* Prototypes */ int discover_nfs_mount_data_version(int *string_ver); void print_one(char *spec, char *node, char *type, char *opts); void mount_usage(void); void umount_usage(void); int chk_mountpoint(const char *mount_point); +char *utf8str2mbstr(const char *utf8_str); +int is_spec_nfs_url(const char *spec); int nfs_umount23(const char *devname, char *string); +int mount_parse_nfs_url(const char *spec, parsed_nfs_url *pnu); +void mount_free_parse_nfs_url(parsed_nfs_url *pnu); + #endif /* !_NFS_UTILS_MOUNT_UTILS_H */ -- 2.30.2