[PATCH] Check for beginning '/' in the mount path

[PATCH] Check for beginning '/' in the mount path

[Malahal Naineni]

NFSv4 gladly accepts and mounts "hostname:path" instead of
"hostname:/path". This causes mount entry mistmatch between /etc/mtab
and /proc/mounts files. The former will have "hostname:path" but the
latter will have "hostname:/path". This causes umount not work at all.

Signed-off-by: Malahal Naineni <malahal@us.ibm.com>
---
 utils/mount/stropts.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index d52e21a..559114d 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -571,6 +571,7 @@ static int nfs_sys_mount(struct nfsmount_info *mi, struct mount_options *opts)
 {
 	char *options = NULL;
 	int result;
+	char *dirname;
 
 	if (mi->fake)
 		return 1;
@@ -580,6 +581,27 @@ static int nfs_sys_mount(struct nfsmount_info *mi, struct mount_options *opts)
 		return 0;
 	}
 
+	/*
+	 * Make sure that dirname in the spec starts with '/'. The mount
+	 * works fine in NFSv4 without the '/', but then /etc/mtab and
+	 * /proc/mounts disagree on the actual mount entry causing later
+	 * umount to fail! There is no reason to allow any dirname not
+	 * starting with '/', so fail it here.
+	 *
+	 * Note that for NFSv2 and NFSv3, the mount call itself fails if
+	 * dirname doesn't start with '/'. So this check is only useful
+	 * for NFSv4.
+	 */
+	dirname = strchr(mi->spec, ']');	/* IPv6 address check */
+	if (dirname)
+		dirname = strchr(dirname+1, ':');
+	else
+		dirname = strchr(mi->spec, ':');
+	if (!dirname || *(dirname+1) != '/') {
+		errno = EINVAL;
+		return 0;
+	}
+
 	result = mount(mi->spec, mi->node, mi->type,
 			mi->flags & ~(MS_USER|MS_USERS), options);
 	free(options);
-- 
1.7.8.3

Re: [PATCH] Check for beginning '/' in the mount path

[Boaz Harrosh]

On 02/03/2012 03:42 AM, Malahal Naineni wrote:
> NFSv4 gladly accepts and mounts "hostname:path" instead of
> "hostname:/path". This causes mount entry mistmatch between /etc/mtab
> and /proc/mounts files. The former will have "hostname:path" but the
> latter will have "hostname:/path". This causes umount not work at all.
> 

NACK

like it or not you are changing ABI. Bunch of systems will not work now.

Also some other NFS servers/clients support it fine. Actually some servers
make it a special case. (It's called mount by tag)

The bug is else where fix it there. Either add the preceding '/' to
/etc/mtab or remove it from /proc/mounts (I prefer the later). Or
fix umount to work with that case.

It's just a bug please fix it.

Boaz

> Signed-off-by: Malahal Naineni <malahal@us.ibm.com>
> ---
>  utils/mount/stropts.c |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index d52e21a..559114d 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -571,6 +571,7 @@ static int nfs_sys_mount(struct nfsmount_info *mi, struct mount_options *opts)
>  {
>  	char *options = NULL;
>  	int result;
> +	char *dirname;
>  
>  	if (mi->fake)
>  		return 1;
> @@ -580,6 +581,27 @@ static int nfs_sys_mount(struct nfsmount_info *mi, struct mount_options *opts)
>  		return 0;
>  	}
>  
> +	/*
> +	 * Make sure that dirname in the spec starts with '/'. The mount
> +	 * works fine in NFSv4 without the '/', but then /etc/mtab and
> +	 * /proc/mounts disagree on the actual mount entry causing later
> +	 * umount to fail! There is no reason to allow any dirname not
> +	 * starting with '/', so fail it here.
> +	 *
> +	 * Note that for NFSv2 and NFSv3, the mount call itself fails if
> +	 * dirname doesn't start with '/'. So this check is only useful
> +	 * for NFSv4.
> +	 */
> +	dirname = strchr(mi->spec, ']');	/* IPv6 address check */
> +	if (dirname)
> +		dirname = strchr(dirname+1, ':');
> +	else
> +		dirname = strchr(mi->spec, ':');
> +	if (!dirname || *(dirname+1) != '/') {
> +		errno = EINVAL;
> +		return 0;
> +	}
> +
>  	result = mount(mi->spec, mi->node, mi->type,
>  			mi->flags & ~(MS_USER|MS_USERS), options);
>  	free(options);

Re: [PATCH] Check for beginning '/' in the mount path

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1098 bytes --]

On Fri, 3 Feb 2012 13:15:12 +0200 Boaz Harrosh <bharrosh@panasas.com> wrote:

> On 02/03/2012 03:42 AM, Malahal Naineni wrote:
> > NFSv4 gladly accepts and mounts "hostname:path" instead of
> > "hostname:/path". This causes mount entry mistmatch between /etc/mtab
> > and /proc/mounts files. The former will have "hostname:path" but the
> > latter will have "hostname:/path". This causes umount not work at all.
> > 
> 
> NACK
> 
> like it or not you are changing ABI. Bunch of systems will not work now.
> 
> Also some other NFS servers/clients support it fine. Actually some servers
> make it a special case. (It's called mount by tag)
> 
> The bug is else where fix it there. Either add the preceding '/' to
> /etc/mtab or remove it from /proc/mounts (I prefer the later). Or
                                            ^^^^^^^^^^^^^^^^^^
> fix umount to work with that case.

Agreed.  And while we are at it we should remove the trailing '/' too.
If you
  mount host:/path /somewhere
/proc/mounts will show
    host:/path/

which also confused mount.

NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] Check for beginning '/' in the mount path

[Malahal Naineni]

NeilBrown [neilb@suse.de] wrote:
> On Fri, 3 Feb 2012 13:15:12 +0200 Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
> > On 02/03/2012 03:42 AM, Malahal Naineni wrote:
> > > NFSv4 gladly accepts and mounts "hostname:path" instead of
> > > "hostname:/path". This causes mount entry mistmatch between /etc/mtab
> > > and /proc/mounts files. The former will have "hostname:path" but the
> > > latter will have "hostname:/path". This causes umount not work at all.
> > > 
> > 
> > NACK
> > 
> > like it or not you are changing ABI. Bunch of systems will not work now.
> > 
> > Also some other NFS servers/clients support it fine. Actually some servers
> > make it a special case. (It's called mount by tag)
> > 
> > The bug is else where fix it there. Either add the preceding '/' to
> > /etc/mtab or remove it from /proc/mounts (I prefer the later). Or
>                                             ^^^^^^^^^^^^^^^^^^
> > fix umount to work with that case.
> 
> Agreed.  And while we are at it we should remove the trailing '/' too.
> If you
>   mount host:/path /somewhere
> /proc/mounts will show
>     host:/path/
> 
> which also confused mount.

Thank you Neil and Boaz. Since there is already a fix in umount to take
care of trailing slash, I will post a patch to take care of leading
slash.

Any reason(s) why trailing slash is fixed in umount rather than in
/proc/mounts?

Thanks, Malahal.

Re: [PATCH] Check for beginning '/' in the mount path

[Boaz Harrosh]

On 02/03/2012 04:29 PM, Malahal Naineni wrote:
> NeilBrown [neilb@suse.de] wrote:
>> On Fri, 3 Feb 2012 13:15:12 +0200 Boaz Harrosh <bharrosh@panasas.com> wrote:
>>
>>> On 02/03/2012 03:42 AM, Malahal Naineni wrote:
>>>> NFSv4 gladly accepts and mounts "hostname:path" instead of
>>>> "hostname:/path". This causes mount entry mistmatch between /etc/mtab
>>>> and /proc/mounts files. The former will have "hostname:path" but the
>>>> latter will have "hostname:/path". This causes umount not work at all.
>>>>
>>>
>>> NACK
>>>
>>> like it or not you are changing ABI. Bunch of systems will not work now.
>>>
>>> Also some other NFS servers/clients support it fine. Actually some servers
>>> make it a special case. (It's called mount by tag)
>>>
>>> The bug is else where fix it there. Either add the preceding '/' to
>>> /etc/mtab or remove it from /proc/mounts (I prefer the later). Or
>>                                             ^^^^^^^^^^^^^^^^^^
>>> fix umount to work with that case.
>>
>> Agreed.  And while we are at it we should remove the trailing '/' too.
>> If you
>>   mount host:/path /somewhere
>> /proc/mounts will show
>>     host:/path/
>>
>> which also confused mount.
> 
> Thank you Neil and Boaz. Since there is already a fix in umount to take
> care of trailing slash, I will post a patch to take care of leading
> slash.
> 
> Any reason(s) why trailing slash is fixed in umount rather than in
> /proc/mounts?
> 

Thank you Malahal.

I think that both fixes to umount are important for future/backward compatibility
with Kernel, And general code resilience. 

But some kind sole should fix both breakages in /proc/mounts, for sure.
Would you know how to fix it?

> Thanks, Malahal.
> 

Thanks
Boaz

Re: [PATCH] Check for beginning '/' in the mount path

[Malahal Naineni]

Boaz Harrosh [bharrosh@panasas.com] wrote:
> I think that both fixes to umount are important for future/backward
> compatibility with Kernel, And general code resilience. 
> 
> But some kind sole should fix both breakages in /proc/mounts, for
> sure.  Would you know how to fix it?

I guess, fixing the umount is needed and it is easy. I will send a fix.

The kernel side is not as easy! The NFS4 mounts the root first then the
exported path using subtree_mount() which seem to remove the actual
string and replace it with a real path. We can save the devname
somewhere and get it into nfs_show_devname(), but I am not sure that
works.

Thanks, Malahal.

[PATCH] Get normalized paths for comparing NFS export paths

[Malahal Naineni]

NFSv4 gladly accepts and mounts "hostname:path" instead of
"hostname:/path". NFS also accepts other forms for pathname, but it
doesn't use the exact pathname string used for generating /proc/mounts.
This causes mount entry mistmatch between /etc/mtab and /proc/mounts
files. The former will have the exact given pathname string but the
latter will have a modified path name.

Signed-off-by: Malahal Naineni <malahal@us.ibm.com>
---
 utils/mount/nfsumount.c |   94 +++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 83 insertions(+), 11 deletions(-)

diff --git a/utils/mount/nfsumount.c b/utils/mount/nfsumount.c
index 3538d88..54542a5 100644
--- a/utils/mount/nfsumount.c
+++ b/utils/mount/nfsumount.c
@@ -139,6 +139,88 @@ static int del_mtab(const char *spec, const char *node)
 	return EX_FILEIO;
 }
 
+/* 
+ * Return normalized path.
+ *
+ * Resolve "." and ".." components. Replace multiple slashes with one
+ * slash. realpath is close but doesn't work for us as the path won't
+ * exist on the client.
+ *
+ * The return string must be freed by the caller.
+ */
+static char *normpath(const char *path)
+{
+	const char *ptr, *next, *end;
+	char *norm; /* result */
+
+	if (!path)
+		return NULL;
+
+	norm = malloc(strlen(path)+1);
+	if (!norm)
+		return NULL;
+		
+	end = path+strlen(path);
+	*norm = '\0'; /* Make it a NULL string */
+	for (ptr = path; ptr < end; ptr = next+1) {
+		next = strchr(ptr, '/');
+		if (!next)
+			next = end;
+		int pclen = next - ptr; /* path component length */
+		if (strncmp(ptr, ".", pclen) == 0)
+			continue;
+		if (strncmp(ptr, "..", pclen) == 0) {
+			char *tmp = strrchr(norm, '/');
+			if (tmp)
+				*tmp = '\0';
+			continue;
+		}
+
+		/* Add the new component */
+		strncat(norm, "/", 1);
+		strncat(norm, ptr, pclen);
+	}
+
+	/*
+	 * If there was no component to copy, norm would be null.
+	 * Return "/" in that case
+	 */
+	if (*norm == '\0')
+		strcpy(norm, "/");
+
+	return norm;
+}
+
+/*
+ * Detect if the given two entries refer to the same mount entry.
+ * Usually one entry is from /etc/mtab and the other is from
+ * /proc/mounts.
+ */
+static int nfs_same_mount_entry(const struct mntentchn *mc1,
+				const struct mntentchn *mc2)
+{
+	char *host1, *host2;
+	char *path1, *path2;
+	char *norm1, *norm2;
+	int retval;
+
+	nfs_parse_devname(mc1->m.mnt_fsname, &host1, &path1);
+	nfs_parse_devname(mc2->m.mnt_fsname, &host2, &path2);
+	norm1 = normpath(path1);
+	norm2 = normpath(path2);
+
+	retval = strcmp(host1, host2) == 0 && strcmp(norm1, norm2) == 0;
+
+	free(host1);
+	free(host2);
+	free(path1);
+	free(path2);
+	free(norm1);
+	free(norm2);
+
+	return retval;
+}
+
 /*
  * Detect NFSv4 mounts.
  *
@@ -161,17 +243,7 @@ static int nfs_umount_is_vers4(const struct mntentchn *mc)
 		goto not_found;
 
 	do {
-		size_t nlen = strlen(pmc->m.mnt_fsname);
-
-		/*
-		 * It's possible the mount location string in /proc/mounts
-		 * ends with a '/'. In this case, if the entry came from
-		 * /etc/mtab, it won't have the trailing '/' so deal with
-		 * it.
-		 */
-		while (pmc->m.mnt_fsname[nlen - 1] == '/')
-			nlen--;
-		if (strncmp(pmc->m.mnt_fsname, mc->m.mnt_fsname, nlen) != 0)
+		if (!nfs_same_mount_entry(pmc, mc))
 			continue;
 
 		if (strcmp(pmc->m.mnt_type, "nfs4") == 0)
-- 
1.7.8.3

Re: [PATCH] Get normalized paths for comparing NFS export paths

[Malahal Naineni]

Any comments on the patch below?

Regards, Malahal.

Malahal Naineni [malahal@us.ibm.com] wrote:
> NFSv4 gladly accepts and mounts "hostname:path" instead of
> "hostname:/path". NFS also accepts other forms for pathname, but it
> doesn't use the exact pathname string used for generating /proc/mounts.
> This causes mount entry mistmatch between /etc/mtab and /proc/mounts
> files. The former will have the exact given pathname string but the
> latter will have a modified path name.
> 
> Signed-off-by: Malahal Naineni <malahal@us.ibm.com>
> ---
>  utils/mount/nfsumount.c |   94 +++++++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 83 insertions(+), 11 deletions(-)
> 
> diff --git a/utils/mount/nfsumount.c b/utils/mount/nfsumount.c
> index 3538d88..54542a5 100644
> --- a/utils/mount/nfsumount.c
> +++ b/utils/mount/nfsumount.c
> @@ -139,6 +139,88 @@ static int del_mtab(const char *spec, const char *node)
>  	return EX_FILEIO;
>  }
> 
> +/* 
> + * Return normalized path.
> + *
> + * Resolve "." and ".." components. Replace multiple slashes with one
> + * slash. realpath is close but doesn't work for us as the path won't
> + * exist on the client.
> + *
> + * The return string must be freed by the caller.
> + */
> +static char *normpath(const char *path)
> +{
> +	const char *ptr, *next, *end;
> +	char *norm; /* result */
> +
> +	if (!path)
> +		return NULL;
> +
> +	norm = malloc(strlen(path)+1);
> +	if (!norm)
> +		return NULL;
> +		
> +	end = path+strlen(path);
> +	*norm = '\0'; /* Make it a NULL string */
> +	for (ptr = path; ptr < end; ptr = next+1) {
> +		next = strchr(ptr, '/');
> +		if (!next)
> +			next = end;
> +		int pclen = next - ptr; /* path component length */
> +		if (strncmp(ptr, ".", pclen) == 0)
> +			continue;
> +		if (strncmp(ptr, "..", pclen) == 0) {
> +			char *tmp = strrchr(norm, '/');
> +			if (tmp)
> +				*tmp = '\0';
> +			continue;
> +		}
> +
> +		/* Add the new component */
> +		strncat(norm, "/", 1);
> +		strncat(norm, ptr, pclen);
> +	}
> +
> +	/*
> +	 * If there was no component to copy, norm would be null.
> +	 * Return "/" in that case
> +	 */
> +	if (*norm == '\0')
> +		strcpy(norm, "/");
> +
> +	return norm;
> +}
> +
> +/*
> + * Detect if the given two entries refer to the same mount entry.
> + * Usually one entry is from /etc/mtab and the other is from
> + * /proc/mounts.
> + */
> +static int nfs_same_mount_entry(const struct mntentchn *mc1,
> +				const struct mntentchn *mc2)
> +{
> +	char *host1, *host2;
> +	char *path1, *path2;
> +	char *norm1, *norm2;
> +	int retval;
> +
> +	nfs_parse_devname(mc1->m.mnt_fsname, &host1, &path1);
> +	nfs_parse_devname(mc2->m.mnt_fsname, &host2, &path2);
> +	norm1 = normpath(path1);
> +	norm2 = normpath(path2);
> +
> +	retval = strcmp(host1, host2) == 0 && strcmp(norm1, norm2) == 0;
> +
> +	free(host1);
> +	free(host2);
> +	free(path1);
> +	free(path2);
> +	free(norm1);
> +	free(norm2);
> +
> +	return retval;
> +}
> +
>  /*
>   * Detect NFSv4 mounts.
>   *
> @@ -161,17 +243,7 @@ static int nfs_umount_is_vers4(const struct mntentchn *mc)
>  		goto not_found;
> 
>  	do {
> -		size_t nlen = strlen(pmc->m.mnt_fsname);
> -
> -		/*
> -		 * It's possible the mount location string in /proc/mounts
> -		 * ends with a '/'. In this case, if the entry came from
> -		 * /etc/mtab, it won't have the trailing '/' so deal with
> -		 * it.
> -		 */
> -		while (pmc->m.mnt_fsname[nlen - 1] == '/')
> -			nlen--;
> -		if (strncmp(pmc->m.mnt_fsname, mc->m.mnt_fsname, nlen) != 0)
> +		if (!nfs_same_mount_entry(pmc, mc))
>  			continue;
> 
>  		if (strcmp(pmc->m.mnt_type, "nfs4") == 0)
> -- 
> 1.7.8.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] Get normalized paths for comparing NFS export paths

[Steve Dickson]

Hello,

First of all my apologizes for dropping this... I saw Boaz's
NACK and never got back to it.. More comments in-line. 

On 02/07/2012 03:44 PM, Malahal Naineni wrote:
> NFSv4 gladly accepts and mounts "hostname:path" instead of
> "hostname:/path". NFS also accepts other forms for pathname, but it
> doesn't use the exact pathname string used for generating /proc/mounts.
> This causes mount entry mistmatch between /etc/mtab and /proc/mounts
> files. The former will have the exact given pathname string but the
> latter will have a modified path name.
> 
> Signed-off-by: Malahal Naineni <malahal@us.ibm.com>
> ---
>  utils/mount/nfsumount.c |   94 +++++++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 83 insertions(+), 11 deletions(-)
> 
> diff --git a/utils/mount/nfsumount.c b/utils/mount/nfsumount.c
> index 3538d88..54542a5 100644
> --- a/utils/mount/nfsumount.c
> +++ b/utils/mount/nfsumount.c
> @@ -139,6 +139,88 @@ static int del_mtab(const char *spec, const char *node)
>  	return EX_FILEIO;
>  }
>  
> +/* 
> + * Return normalized path.
> + *
> + * Resolve "." and ".." components. Replace multiple slashes with one
> + * slash. realpath is close but doesn't work for us as the path won't
> + * exist on the client.
> + *
> + * The return string must be freed by the caller.
> + */
> +static char *normpath(const char *path)
> +{
> +	const char *ptr, *next, *end;
> +	char *norm; /* result */
> +
> +	if (!path)
> +		return NULL;
> +
> +	norm = malloc(strlen(path)+1);
> +	if (!norm)
> +		return NULL;
> +		
> +	end = path+strlen(path);
> +	*norm = '\0'; /* Make it a NULL string */
> +	for (ptr = path; ptr < end; ptr = next+1) {
> +		next = strchr(ptr, '/');
> +		if (!next)
> +			next = end;
> +		int pclen = next - ptr; /* path component length */
> +		if (strncmp(ptr, ".", pclen) == 0)
> +			continue;
> +		if (strncmp(ptr, "..", pclen) == 0) {
> +			char *tmp = strrchr(norm, '/');
> +			if (tmp)
> +				*tmp = '\0';
> +			continue;
> +		}
> +
> +		/* Add the new component */
> +		strncat(norm, "/", 1);
> +		strncat(norm, ptr, pclen);
> +	}
> +
> +	/*
> +	 * If there was no component to copy, norm would be null.
> +	 * Return "/" in that case
> +	 */
> +	if (*norm == '\0')
> +		strcpy(norm, "/");
> +
> +	return norm;
> +}
> +
> +/*
> + * Detect if the given two entries refer to the same mount entry.
> + * Usually one entry is from /etc/mtab and the other is from
> + * /proc/mounts.
> + */
> +static int nfs_same_mount_entry(const struct mntentchn *mc1,
> +				const struct mntentchn *mc2)
> +{
> +	char *host1, *host2;
> +	char *path1, *path2;
> +	char *norm1, *norm2;
> +	int retval;
> +
> +	nfs_parse_devname(mc1->m.mnt_fsname, &host1, &path1);
> +	nfs_parse_devname(mc2->m.mnt_fsname, &host2, &path2);
> +	norm1 = normpath(path1);
> +	norm2 = normpath(path2);
> +
> +	retval = strcmp(host1, host2) == 0 && strcmp(norm1, norm2) == 0;
> +
> +	free(host1);
> +	free(host2);
> +	free(path1);
> +	free(path2);
> +	free(norm1);
> +	free(norm2);
> +
> +	return retval;
> +}
> +
>  /*
>   * Detect NFSv4 mounts.
>   *
> @@ -161,17 +243,7 @@ static int nfs_umount_is_vers4(const struct mntentchn *mc)
>  		goto not_found;
>  
>  	do {
> -		size_t nlen = strlen(pmc->m.mnt_fsname);
> -
> -		/*
> -		 * It's possible the mount location string in /proc/mounts
> -		 * ends with a '/'. In this case, if the entry came from
> -		 * /etc/mtab, it won't have the trailing '/' so deal with
> -		 * it.
> -		 */
> -		while (pmc->m.mnt_fsname[nlen - 1] == '/')
> -			nlen--;
> -		if (strncmp(pmc->m.mnt_fsname, mc->m.mnt_fsname, nlen) != 0)
> +		if (!nfs_same_mount_entry(pmc, mc))
>  			continue;
Question, How does this patch stop the mtab from getting polluted with
the device name with multiple "/"? 

steved.

Re: [PATCH] Get normalized paths for comparing NFS export paths

[Malahal Naineni]

Steve Dickson [SteveD@redhat.com] wrote:
> Hello,
> 
> First of all my apologizes for dropping this... I saw Boaz's
> NACK and never got back to it.. More comments in-line. 

No need for them.

> Question, How does this patch stop the mtab from getting polluted with
> the device name with multiple "/"? 

It doesn't. Is that a problem? It only matches entries in /etc/mtab and
/proc/mounts correctly for unmount to work.

Regards, Malahal.

Re: [PATCH] Get normalized paths for comparing NFS export paths

[Steve Dickson]

On 03/02/2012 02:27 PM, Malahal Naineni wrote:
>> Question, How does this patch stop the mtab from getting polluted with
>> the device name with multiple "/"? 
> 
> It doesn't. Is that a problem? It only matches entries in /etc/mtab and
> /proc/mounts correctly for unmount to work.
> 
Well the problem I was working on was when a mount like
the following is done:
    mount server://export /mnt 

the umount /mnt will fail because the device name was written to 
/proc/mounts has the // stripped:

    server:/export /mnt nfs4 rw,relatime,vers=4...
 
but the device name in the mtab did not:

    rhelhat://home /mnt/home nfs rw,vers=4,... 

So what my patch does is "normalizes" the device name early
on in main, so the correct name used used through the mount
and when its written the mtab. Plus, for better or worses, 
since the new device name will always be shorter, I just 
reuse/rewrite the memory allocated for the argv vector.. 
Meaning there is no allocation... 

steved.

Re: [PATCH] Get normalized paths for comparing NFS export paths

[Malahal Naineni]

Steve Dickson [SteveD@redhat.com] wrote:
> So what my patch does is "normalizes" the device name early
> on in main, so the correct name used used through the mount
> and when its written the mtab. Plus, for better or worses, 
> since the new device name will always be shorter, I just 
> reuse/rewrite the memory allocated for the argv vector.. 
> Meaning there is no allocation... 

My problem is a bit different.

"mount -t nfs4 server:export /mnt" works but umount fails.

Notice that there is no '/' in the path!

Normalizing or just stripping leading '/'s early won't help with the
above problem and since there is already a hack to strip the
__trailing__ '/' that kernel adds to /proc/mounts file, I just made the
existing hack it a bit better by normalizing.

Regards, Malahal.

Re: [PATCH] Get normalized paths for comparing NFS export paths

[Steve Dickson]

On 03/02/2012 05:01 PM, Malahal Naineni wrote:
> Steve Dickson [SteveD@redhat.com] wrote:
>> So what my patch does is "normalizes" the device name early
>> on in main, so the correct name used used through the mount
>> and when its written the mtab. Plus, for better or worses, 
>> since the new device name will always be shorter, I just 
>> reuse/rewrite the memory allocated for the argv vector.. 
>> Meaning there is no allocation... 
> 
> My problem is a bit different.
> 
> "mount -t nfs4 server:export /mnt" works but umount fails.
> 
> Notice that there is no '/' in the path!
> 
> Normalizing or just stripping leading '/'s early won't help with the
> above problem and since there is already a hack to strip the
> __trailing__ '/' that kernel adds to /proc/mounts file, I just made the
> existing hack it a bit better by normalizing.
>
How about something like this... It takes on both case early on...

Author: Steve Dickson <steved@redhat.com>
Date:   Sat Mar 3 12:31:23 2012 -0500

    mount.nfs: Validate device name syntax
    
    To ensure the device name is found at unmount time strip
    off the multiple '/' or add a '/' if one does not exist.
    
    Signed-off-by: Steve Dickson <steved@redhat.com>

diff --git a/utils/mount/mount.c b/utils/mount/mount.c
index eea00af..7b9ba8b 100644
--- a/utils/mount/mount.c
+++ b/utils/mount/mount.c
@@ -344,6 +344,52 @@ static void parse_opts(const char *options, int *flags, char **extra_opts)
 	}
 }
 
+/*
+ * To ensure the device is found at unmount time strip
+ * off the multiple '/' or add a '/' if one does not exist.
+ */
+static char *
+chk_devicename(char *spec) {
+
+	char *colen, *slash;
+
+	if (strstr(spec, "//") != NULL) {
+		if ((colen = strchr(spec, ':')) == NULL)
+			return NULL;
+
+		slash = (colen + 1);
+		while (*slash && *(slash+1) == '/')
+			slash++;
+		while (*slash)
+			*(++colen) = *(slash++);
+		*(colen+1) = '\0';
+
+		return spec;
+	} 
+
+	if (strchr(spec, '/') == NULL) {
+		char *colen, *ptr, *str, *dev;
+		
+		if ((colen = strchr(spec, ':')) == NULL)
+			return NULL;
+
+		dev = str = malloc(strlen(spec) + 2);
+		if (dev == NULL) 
+			return NULL;
+
+		ptr = spec;
+		while (ptr <= colen)
+			*(str++) = *(ptr++);
+		*str++='/';
+		while (*ptr)
+			*(str++) = *(ptr++);
+		*str='\0';
+		spec = dev;
+	}
+
+	return spec;
+}
+
 static int try_mount(char *spec, char *mount_point, int flags,
 			char *fs_type, char **extra_opts, char *mount_opts,
 			int fake, int bg)
@@ -445,6 +491,15 @@ int main(int argc, char *argv[])
 		}
 	}
 
+	/*
+	 * Validate the device name syntax
+	 */
+	if ((spec = chk_devicename(argv[1])) == NULL) {
+		nfs_error(_("%s: %s - Invalid device name syntax"),
+			progname, argv[1]);
+		goto out_usage;
+	}
+
 	if (strcmp(progname, "mount.nfs4") == 0)
 		fs_type = "nfs4";
 

Re: [PATCH] Get normalized paths for comparing NFS export paths

[Myklebust, Trond]

T24gU2F0LCAyMDEyLTAzLTAzIGF0IDEyOjM5IC0wNTAwLCBTdGV2ZSBEaWNrc29uIHdyb3RlOg0K
PiANCj4gT24gMDMvMDIvMjAxMiAwNTowMSBQTSwgTWFsYWhhbCBOYWluZW5pIHdyb3RlOg0KPiA+
IFN0ZXZlIERpY2tzb24gW1N0ZXZlREByZWRoYXQuY29tXSB3cm90ZToNCj4gPj4gU28gd2hhdCBt
eSBwYXRjaCBkb2VzIGlzICJub3JtYWxpemVzIiB0aGUgZGV2aWNlIG5hbWUgZWFybHkNCj4gPj4g
b24gaW4gbWFpbiwgc28gdGhlIGNvcnJlY3QgbmFtZSB1c2VkIHVzZWQgdGhyb3VnaCB0aGUgbW91
bnQNCj4gPj4gYW5kIHdoZW4gaXRzIHdyaXR0ZW4gdGhlIG10YWIuIFBsdXMsIGZvciBiZXR0ZXIg
b3Igd29yc2VzLCANCj4gPj4gc2luY2UgdGhlIG5ldyBkZXZpY2UgbmFtZSB3aWxsIGFsd2F5cyBi
ZSBzaG9ydGVyLCBJIGp1c3QgDQo+ID4+IHJldXNlL3Jld3JpdGUgdGhlIG1lbW9yeSBhbGxvY2F0
ZWQgZm9yIHRoZSBhcmd2IHZlY3Rvci4uIA0KPiA+PiBNZWFuaW5nIHRoZXJlIGlzIG5vIGFsbG9j
YXRpb24uLi4gDQo+ID4gDQo+ID4gTXkgcHJvYmxlbSBpcyBhIGJpdCBkaWZmZXJlbnQuDQo+ID4g
DQo+ID4gIm1vdW50IC10IG5mczQgc2VydmVyOmV4cG9ydCAvbW50IiB3b3JrcyBidXQgdW1vdW50
IGZhaWxzLg0KPiA+IA0KPiA+IE5vdGljZSB0aGF0IHRoZXJlIGlzIG5vICcvJyBpbiB0aGUgcGF0
aCENCj4gPiANCj4gPiBOb3JtYWxpemluZyBvciBqdXN0IHN0cmlwcGluZyBsZWFkaW5nICcvJ3Mg
ZWFybHkgd29uJ3QgaGVscCB3aXRoIHRoZQ0KPiA+IGFib3ZlIHByb2JsZW0gYW5kIHNpbmNlIHRo
ZXJlIGlzIGFscmVhZHkgYSBoYWNrIHRvIHN0cmlwIHRoZQ0KPiA+IF9fdHJhaWxpbmdfXyAnLycg
dGhhdCBrZXJuZWwgYWRkcyB0byAvcHJvYy9tb3VudHMgZmlsZSwgSSBqdXN0IG1hZGUgdGhlDQo+
ID4gZXhpc3RpbmcgaGFjayBpdCBhIGJpdCBiZXR0ZXIgYnkgbm9ybWFsaXppbmcuDQo+ID4NCj4g
SG93IGFib3V0IHNvbWV0aGluZyBsaWtlIHRoaXMuLi4gSXQgdGFrZXMgb24gYm90aCBjYXNlIGVh
cmx5IG9uLi4uDQo+IA0KPiBBdXRob3I6IFN0ZXZlIERpY2tzb24gPHN0ZXZlZEByZWRoYXQuY29t
Pg0KPiBEYXRlOiAgIFNhdCBNYXIgMyAxMjozMToyMyAyMDEyIC0wNTAwDQo+IA0KPiAgICAgbW91
bnQubmZzOiBWYWxpZGF0ZSBkZXZpY2UgbmFtZSBzeW50YXgNCj4gICAgIA0KPiAgICAgVG8gZW5z
dXJlIHRoZSBkZXZpY2UgbmFtZSBpcyBmb3VuZCBhdCB1bm1vdW50IHRpbWUgc3RyaXANCj4gICAg
IG9mZiB0aGUgbXVsdGlwbGUgJy8nIG9yIGFkZCBhICcvJyBpZiBvbmUgZG9lcyBub3QgZXhpc3Qu
DQo+ICAgICANCj4gICAgIFNpZ25lZC1vZmYtYnk6IFN0ZXZlIERpY2tzb24gPHN0ZXZlZEByZWRo
YXQuY29tPg0KDQpOQUNLLg0KDQpORlN2NCBpcyB0aGUgb25seSBwcm90b2NvbCB0aGF0IGhhcyBh
IHN0YW5kYXJkIG1vdW50IHBhdGggc3ludGF4LCBhbmQNCnRoYXQgaXMgYmVjYXVzZSB0aGUgY2xp
ZW50IHBlcmZvcm1zIHRoZSBqb2Igb2YgaW50ZXJwcmV0aW5nIHRoZSBwYXRoDQpuYW1lIGFuZCB0
cmFuc2xhdGluZyBpdCBpbnRvIFBVVFJPT1RGSCBmb2xsb3dlZCBieSBhIGJ1bmNoIG9mIExPT0tV
UHMuDQpJT1c6IHRoZSBzdGFuZGFyZCBzeW50YXggdGhlcmUgaXMgdGhlIG9uZSBpbXBvc2VkIGJ5
IHRoZSBjbGllbnQuDQoNClRoZXJlIGlzIG5vdGhpbmcgaW4gdGhlIE5GU3YyL3YzIE1PVU5UIHNw
ZWMgdGhhdCBzdGF0ZXMgdGhhdCBhIHBhdGgNCm5lZWRzIHRvIHN0YXJ0IHdpdGggJy8nLiBOb3Ig
aXMgdGhlcmUgZXZlbiBhbnl0aGluZyBpbiB0aGUgc3BlYyB0aGF0DQpzdGF0ZXMgdGhhdCAnLycg
aXMgcmVxdWlyZWQgdG8gYmUgdXNlZCBhcyB0aGUgZGlyZWN0b3J5IGNvbXBvbmVudA0Kc2VwYXJh
dG9yLiBUaGUgWC9PUEVOIGRvY3Mgc3RhdGUgdGhhdCAnLycgaXMgcmVjb21tZW5kZWQgZm9yDQpw
b3J0YWJpbGl0eSwgYnV0IGRvIG5vdCBtYWtlIGl0IGEgcmVxdWlyZW1lbnQuIFNlZQ0KaHR0cDov
L3B1YnMub3Blbmdyb3VwLm9yZy9vbmxpbmVwdWJzLzk2Mjk3OTkvY2hhcDguaHRtI3RhZ2NqaF8w
OV8wMl8wMl8wMyANCg0KSU9XOiBJJ20gcGVyZmVjdGx5IGFsbG93ZWQgdG8gc2V0IHVwIGEgJ21v
dW50ZCcgc2VydmVyIHRoYXQgdXNlcyAnXCcgb3INCmV2ZW4gc29tZXRoaW5nIGxpa2UgJ3wnIGFz
IGEgcGF0aCBjb21wb25lbnQgc2VwYXJhdG9yLiBUaGlzIGtpbmQgb2YNCnBhdGNoIHdvdWxkIGJy
ZWFrIHRoZSBjbGllbnQncyBleGlzdGluZyBhYmlsaXR5IHRvIG1vdW50IGZyb20gc3VjaCBhDQpz
ZXJ2ZXIuDQoNCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRh
aW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cubmV0YXBwLmNv
bQ0KDQo=

Re: [PATCH] Get normalized paths for comparing NFS export paths

[Steve Dickson]

On 03/03/2012 02:12 PM, Myklebust, Trond wrote:
> On Sat, 2012-03-03 at 12:39 -0500, Steve Dickson wrote:
>>
>> On 03/02/2012 05:01 PM, Malahal Naineni wrote:
>>> Steve Dickson [SteveD@redhat.com] wrote:
>>>> So what my patch does is "normalizes" the device name early
>>>> on in main, so the correct name used used through the mount
>>>> and when its written the mtab. Plus, for better or worses, 
>>>> since the new device name will always be shorter, I just 
>>>> reuse/rewrite the memory allocated for the argv vector.. 
>>>> Meaning there is no allocation... 
>>>
>>> My problem is a bit different.
>>>
>>> "mount -t nfs4 server:export /mnt" works but umount fails.
>>>
>>> Notice that there is no '/' in the path!
>>>
>>> Normalizing or just stripping leading '/'s early won't help with the
>>> above problem and since there is already a hack to strip the
>>> __trailing__ '/' that kernel adds to /proc/mounts file, I just made the
>>> existing hack it a bit better by normalizing.
>>>
>> How about something like this... It takes on both case early on...
>>
>> Author: Steve Dickson <steved@redhat.com>
>> Date:   Sat Mar 3 12:31:23 2012 -0500
>>
>>     mount.nfs: Validate device name syntax
>>     
>>     To ensure the device name is found at unmount time strip
>>     off the multiple '/' or add a '/' if one does not exist.
>>     
>>     Signed-off-by: Steve Dickson <steved@redhat.com>
> 
> NACK.
> 
> NFSv4 is the only protocol that has a standard mount path syntax, and
> that is because the client performs the job of interpreting the path
> name and translating it into PUTROOTFH followed by a bunch of LOOKUPs.
> IOW: the standard syntax there is the one imposed by the client.
> 
> There is nothing in the NFSv2/v3 MOUNT spec that states that a path
> needs to start with '/'. Nor is there even anything in the spec that
> states that '/' is required to be used as the directory component
> separator. The X/OPEN docs state that '/' is recommended for
> portability, but do not make it a requirement. See
> http://pubs.opengroup.org/onlinepubs/9629799/chap8.htm#tagcjh_09_02_02_03 
> 
> IOW: I'm perfectly allowed to set up a 'mountd' server that uses '\' or
> even something like '|' as a path component separator. This kind of
> patch would break the client's existing ability to mount from such a
> server.
And where does an server like this exist? One that uses '|' as its
path component separator?? ;-)

Just to be clear, you are ok with striping the multiple slashes, for
all protocol versions, but its only kosher to added the leading 
slash for v4 mounts. Correct?  

steved.
 

Re: [PATCH] Get normalized paths for comparing NFS export paths

[Myklebust, Trond]

T24gU3VuLCAyMDEyLTAzLTA0IGF0IDE3OjMxIC0wNTAwLCBTdGV2ZSBEaWNrc29uIHdyb3RlOg0K
PiANCj4gT24gMDMvMDMvMjAxMiAwMjoxMiBQTSwgTXlrbGVidXN0LCBUcm9uZCB3cm90ZToNCj4g
PiBPbiBTYXQsIDIwMTItMDMtMDMgYXQgMTI6MzkgLTA1MDAsIFN0ZXZlIERpY2tzb24gd3JvdGU6
DQo+ID4+DQo+ID4+IE9uIDAzLzAyLzIwMTIgMDU6MDEgUE0sIE1hbGFoYWwgTmFpbmVuaSB3cm90
ZToNCj4gPj4+IFN0ZXZlIERpY2tzb24gW1N0ZXZlREByZWRoYXQuY29tXSB3cm90ZToNCj4gPj4+
PiBTbyB3aGF0IG15IHBhdGNoIGRvZXMgaXMgIm5vcm1hbGl6ZXMiIHRoZSBkZXZpY2UgbmFtZSBl
YXJseQ0KPiA+Pj4+IG9uIGluIG1haW4sIHNvIHRoZSBjb3JyZWN0IG5hbWUgdXNlZCB1c2VkIHRo
cm91Z2ggdGhlIG1vdW50DQo+ID4+Pj4gYW5kIHdoZW4gaXRzIHdyaXR0ZW4gdGhlIG10YWIuIFBs
dXMsIGZvciBiZXR0ZXIgb3Igd29yc2VzLCANCj4gPj4+PiBzaW5jZSB0aGUgbmV3IGRldmljZSBu
YW1lIHdpbGwgYWx3YXlzIGJlIHNob3J0ZXIsIEkganVzdCANCj4gPj4+PiByZXVzZS9yZXdyaXRl
IHRoZSBtZW1vcnkgYWxsb2NhdGVkIGZvciB0aGUgYXJndiB2ZWN0b3IuLiANCj4gPj4+PiBNZWFu
aW5nIHRoZXJlIGlzIG5vIGFsbG9jYXRpb24uLi4gDQo+ID4+Pg0KPiA+Pj4gTXkgcHJvYmxlbSBp
cyBhIGJpdCBkaWZmZXJlbnQuDQo+ID4+Pg0KPiA+Pj4gIm1vdW50IC10IG5mczQgc2VydmVyOmV4
cG9ydCAvbW50IiB3b3JrcyBidXQgdW1vdW50IGZhaWxzLg0KPiA+Pj4NCj4gPj4+IE5vdGljZSB0
aGF0IHRoZXJlIGlzIG5vICcvJyBpbiB0aGUgcGF0aCENCj4gPj4+DQo+ID4+PiBOb3JtYWxpemlu
ZyBvciBqdXN0IHN0cmlwcGluZyBsZWFkaW5nICcvJ3MgZWFybHkgd29uJ3QgaGVscCB3aXRoIHRo
ZQ0KPiA+Pj4gYWJvdmUgcHJvYmxlbSBhbmQgc2luY2UgdGhlcmUgaXMgYWxyZWFkeSBhIGhhY2sg
dG8gc3RyaXAgdGhlDQo+ID4+PiBfX3RyYWlsaW5nX18gJy8nIHRoYXQga2VybmVsIGFkZHMgdG8g
L3Byb2MvbW91bnRzIGZpbGUsIEkganVzdCBtYWRlIHRoZQ0KPiA+Pj4gZXhpc3RpbmcgaGFjayBp
dCBhIGJpdCBiZXR0ZXIgYnkgbm9ybWFsaXppbmcuDQo+ID4+Pg0KPiA+PiBIb3cgYWJvdXQgc29t
ZXRoaW5nIGxpa2UgdGhpcy4uLiBJdCB0YWtlcyBvbiBib3RoIGNhc2UgZWFybHkgb24uLi4NCj4g
Pj4NCj4gPj4gQXV0aG9yOiBTdGV2ZSBEaWNrc29uIDxzdGV2ZWRAcmVkaGF0LmNvbT4NCj4gPj4g
RGF0ZTogICBTYXQgTWFyIDMgMTI6MzE6MjMgMjAxMiAtMDUwMA0KPiA+Pg0KPiA+PiAgICAgbW91
bnQubmZzOiBWYWxpZGF0ZSBkZXZpY2UgbmFtZSBzeW50YXgNCj4gPj4gICAgIA0KPiA+PiAgICAg
VG8gZW5zdXJlIHRoZSBkZXZpY2UgbmFtZSBpcyBmb3VuZCBhdCB1bm1vdW50IHRpbWUgc3RyaXAN
Cj4gPj4gICAgIG9mZiB0aGUgbXVsdGlwbGUgJy8nIG9yIGFkZCBhICcvJyBpZiBvbmUgZG9lcyBu
b3QgZXhpc3QuDQo+ID4+ICAgICANCj4gPj4gICAgIFNpZ25lZC1vZmYtYnk6IFN0ZXZlIERpY2tz
b24gPHN0ZXZlZEByZWRoYXQuY29tPg0KPiA+IA0KPiA+IE5BQ0suDQo+ID4gDQo+ID4gTkZTdjQg
aXMgdGhlIG9ubHkgcHJvdG9jb2wgdGhhdCBoYXMgYSBzdGFuZGFyZCBtb3VudCBwYXRoIHN5bnRh
eCwgYW5kDQo+ID4gdGhhdCBpcyBiZWNhdXNlIHRoZSBjbGllbnQgcGVyZm9ybXMgdGhlIGpvYiBv
ZiBpbnRlcnByZXRpbmcgdGhlIHBhdGgNCj4gPiBuYW1lIGFuZCB0cmFuc2xhdGluZyBpdCBpbnRv
IFBVVFJPT1RGSCBmb2xsb3dlZCBieSBhIGJ1bmNoIG9mIExPT0tVUHMuDQo+ID4gSU9XOiB0aGUg
c3RhbmRhcmQgc3ludGF4IHRoZXJlIGlzIHRoZSBvbmUgaW1wb3NlZCBieSB0aGUgY2xpZW50Lg0K
PiA+IA0KPiA+IFRoZXJlIGlzIG5vdGhpbmcgaW4gdGhlIE5GU3YyL3YzIE1PVU5UIHNwZWMgdGhh
dCBzdGF0ZXMgdGhhdCBhIHBhdGgNCj4gPiBuZWVkcyB0byBzdGFydCB3aXRoICcvJy4gTm9yIGlz
IHRoZXJlIGV2ZW4gYW55dGhpbmcgaW4gdGhlIHNwZWMgdGhhdA0KPiA+IHN0YXRlcyB0aGF0ICcv
JyBpcyByZXF1aXJlZCB0byBiZSB1c2VkIGFzIHRoZSBkaXJlY3RvcnkgY29tcG9uZW50DQo+ID4g
c2VwYXJhdG9yLiBUaGUgWC9PUEVOIGRvY3Mgc3RhdGUgdGhhdCAnLycgaXMgcmVjb21tZW5kZWQg
Zm9yDQo+ID4gcG9ydGFiaWxpdHksIGJ1dCBkbyBub3QgbWFrZSBpdCBhIHJlcXVpcmVtZW50LiBT
ZWUNCj4gPiBodHRwOi8vcHVicy5vcGVuZ3JvdXAub3JnL29ubGluZXB1YnMvOTYyOTc5OS9jaGFw
OC5odG0jdGFnY2poXzA5XzAyXzAyXzAzIA0KPiA+IA0KPiA+IElPVzogSSdtIHBlcmZlY3RseSBh
bGxvd2VkIHRvIHNldCB1cCBhICdtb3VudGQnIHNlcnZlciB0aGF0IHVzZXMgJ1wnIG9yDQo+ID4g
ZXZlbiBzb21ldGhpbmcgbGlrZSAnfCcgYXMgYSBwYXRoIGNvbXBvbmVudCBzZXBhcmF0b3IuIFRo
aXMga2luZCBvZg0KPiA+IHBhdGNoIHdvdWxkIGJyZWFrIHRoZSBjbGllbnQncyBleGlzdGluZyBh
YmlsaXR5IHRvIG1vdW50IGZyb20gc3VjaCBhDQo+ID4gc2VydmVyLg0KPiBBbmQgd2hlcmUgZG9l
cyBhbiBzZXJ2ZXIgbGlrZSB0aGlzIGV4aXN0PyBPbmUgdGhhdCB1c2VzICd8JyBhcyBpdHMNCj4g
cGF0aCBjb21wb25lbnQgc2VwYXJhdG9yPz8gOy0pDQoNCkl0IHJlYWxseSBkb2Vzbid0IG1hdHRl
ci4gV2UncmUgc3VwcG9zZWQgdG8gYmUgY29kaW5nIHRoaXMgc29ydCBvZiB0aGluZw0KdG8gdGhl
IHNwZWMsIG5vdCB0byBhbiBlc3RpbWF0ZSBvZiBleGlzdGVuY2UuDQoNCj4gSnVzdCB0byBiZSBj
bGVhciwgeW91IGFyZSBvayB3aXRoIHN0cmlwaW5nIHRoZSBtdWx0aXBsZSBzbGFzaGVzLCBmb3IN
Cj4gYWxsIHByb3RvY29sIHZlcnNpb25zLCBidXQgaXRzIG9ubHkga29zaGVyIHRvIGFkZGVkIHRo
ZSBsZWFkaW5nIA0KPiBzbGFzaCBmb3IgdjQgbW91bnRzLiBDb3JyZWN0PyAgDQoNCk5vLiBQbGVh
c2UgZG9uJ3Qgc3RyaXAgdGhlIG11bHRpcGxlIHNsYXNoZXMgZWl0aGVyLiBKdXN0IGxlYXZlIHRo
ZSBwYXRoDQphbG9uZSBhZnRlciB5b3UndmUgc2VwYXJhdGVkIGl0IGZyb20gdGhlIGRldmljZW5h
bWUuDQoNCkl0IGlzIHF1aXRlIE9LIHRvIG5vcm1hbGl6ZSB0aGUgcGF0aCBvbiB0aGUgX2NsaWVu
dF8gc2lkZSAoaS5lLg0KY2hhbmdlIC8vbW50IHRvIC9tbnQgb3Igd2hhdGV2ZXIpIGJ1dCBkb24n
dCB0b3VjaCB0aGUgc2VydmVyIHNpZGUuDQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBO
RlMgY2xpZW50IG1haW50YWluZXINCg0KTmV0QXBwDQpUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNv
bQ0Kd3d3Lm5ldGFwcC5jb20NCg0K

Re: [PATCH] Get normalized paths for comparing NFS export paths

[Steve Dickson]

On 03/04/2012 05:46 PM, Myklebust, Trond wrote:
> On Sun, 2012-03-04 at 17:31 -0500, Steve Dickson wrote:
>>
>> On 03/03/2012 02:12 PM, Myklebust, Trond wrote:
>>> On Sat, 2012-03-03 at 12:39 -0500, Steve Dickson wrote:
>>>>
>>>> On 03/02/2012 05:01 PM, Malahal Naineni wrote:
>>>>> Steve Dickson [SteveD@redhat.com] wrote:
>>>>>> So what my patch does is "normalizes" the device name early
>>>>>> on in main, so the correct name used used through the mount
>>>>>> and when its written the mtab. Plus, for better or worses, 
>>>>>> since the new device name will always be shorter, I just 
>>>>>> reuse/rewrite the memory allocated for the argv vector.. 
>>>>>> Meaning there is no allocation... 
>>>>>
>>>>> My problem is a bit different.
>>>>>
>>>>> "mount -t nfs4 server:export /mnt" works but umount fails.
>>>>>
>>>>> Notice that there is no '/' in the path!
>>>>>
>>>>> Normalizing or just stripping leading '/'s early won't help with the
>>>>> above problem and since there is already a hack to strip the
>>>>> __trailing__ '/' that kernel adds to /proc/mounts file, I just made the
>>>>> existing hack it a bit better by normalizing.
>>>>>
>>>> How about something like this... It takes on both case early on...
>>>>
>>>> Author: Steve Dickson <steved@redhat.com>
>>>> Date:   Sat Mar 3 12:31:23 2012 -0500
>>>>
>>>>     mount.nfs: Validate device name syntax
>>>>     
>>>>     To ensure the device name is found at unmount time strip
>>>>     off the multiple '/' or add a '/' if one does not exist.
>>>>     
>>>>     Signed-off-by: Steve Dickson <steved@redhat.com>
>>>
>>> NACK.
>>>
>>> NFSv4 is the only protocol that has a standard mount path syntax, and
>>> that is because the client performs the job of interpreting the path
>>> name and translating it into PUTROOTFH followed by a bunch of LOOKUPs.
>>> IOW: the standard syntax there is the one imposed by the client.
>>>
>>> There is nothing in the NFSv2/v3 MOUNT spec that states that a path
>>> needs to start with '/'. Nor is there even anything in the spec that
>>> states that '/' is required to be used as the directory component
>>> separator. The X/OPEN docs state that '/' is recommended for
>>> portability, but do not make it a requirement. See
>>> http://pubs.opengroup.org/onlinepubs/9629799/chap8.htm#tagcjh_09_02_02_03 
>>>
>>> IOW: I'm perfectly allowed to set up a 'mountd' server that uses '\' or
>>> even something like '|' as a path component separator. This kind of
>>> patch would break the client's existing ability to mount from such a
>>> server.
>> And where does an server like this exist? One that uses '|' as its
>> path component separator?? ;-)
> 
> It really doesn't matter. We're supposed to be coding this sort of thing
> to the spec, not to an estimate of existence.
> 
>> Just to be clear, you are ok with striping the multiple slashes, for
>> all protocol versions, but its only kosher to added the leading 
>> slash for v4 mounts. Correct?  
> 
> No. Please don't strip the multiple slashes either. Just leave the path
> alone after you've separated it from the devicename.
Well with v4 mounts the kernel code does exactly that. 
A mount like:
   mount server://///export /mnt

turns into a /proc/mounts of 
   server:/export /mnt .... 

which is the reason the umount can not find the mount.

The same with 
   mount server:export /mnt

the /proc/mounts turns into 
   server:/export /mnt... .

which again is why the umount can not find it... 

> 
> It is quite OK to normalize the path on the _client_ side (i.e.
> change //mnt to /mnt or whatever) but don't touch the server side.
On the unmounts if the device name in /etc/mtab and /proc/mounts
don't match then the umount fails... Again this will only happen on
distro where /etc/mtab and /proc/mounts are not symbolically linked.

steved.

Re: [PATCH] Get normalized paths for comparing NFS export paths

[Malahal Naineni]

Myklebust, Trond [Trond.Myklebust@netapp.com] wrote:
> No. Please don't strip the multiple slashes either. Just leave the path
> alone after you've separated it from the devicename.
> 
> It is quite OK to normalize the path on the _client_ side (i.e.
> change //mnt to /mnt or whatever) but don't touch the server side.

As I understand, we can normalize and store it in /etc/mtab and
normalization is done by the kernel for /proc/mounts. In other words,
follow the kernel and make the /etc/mtab entries consistent with the
kernel.

In my patch, I decided to normalize at umount time when we compare the
mtab and proc file entries.

Any opinion on which one is better?

Thanks, Malahal.

Re: [PATCH] Get normalized paths for comparing NFS export paths

[Steve Dickson]

On 03/04/2012 11:46 PM, Malahal Naineni wrote:
> Myklebust, Trond [Trond.Myklebust@netapp.com] wrote:
>> No. Please don't strip the multiple slashes either. Just leave the path
>> alone after you've separated it from the devicename.
>>
>> It is quite OK to normalize the path on the _client_ side (i.e.
>> change //mnt to /mnt or whatever) but don't touch the server side.
> 
> As I understand, we can normalize and store it in /etc/mtab and
> normalization is done by the kernel for /proc/mounts. In other words,
> follow the kernel and make the /etc/mtab entries consistent with the
> kernel.
> 
> In my patch, I decided to normalize at umount time when we compare the
> mtab and proc file entries.
> 
> Any opinion on which one is better?
I'm thinking either before the unmount or after a successful 
v4 mount...

steved.

Re: [PATCH] Get normalized paths for comparing NFS export paths

[Steve Dickson]

On 03/04/2012 05:31 PM, Steve Dickson wrote:
>> NFSv4 is the only protocol that has a standard mount path syntax, and
>> that is because the client performs the job of interpreting the path
>> name and translating it into PUTROOTFH followed by a bunch of LOOKUPs.
>> IOW: the standard syntax there is the one imposed by the client.
>>
>> There is nothing in the NFSv2/v3 MOUNT spec that states that a path
>> needs to start with '/'. Nor is there even anything in the spec that
>> states that '/' is required to be used as the directory component
>> separator. The X/OPEN docs state that '/' is recommended for
>> portability, but do not make it a requirement. See
>> http://pubs.opengroup.org/onlinepubs/9629799/chap8.htm#tagcjh_09_02_02_03 
>>
>> IOW: I'm perfectly allowed to set up a 'mountd' server that uses '\' or
>> even something like '|' as a path component separator. This kind of
>> patch would break the client's existing ability to mount from such a
>> server.
> And where does an server like this exist? One that uses '|' as its
> path component separator?? ;-)
> 
> Just to be clear, you are ok with striping the multiple slashes, for
> all protocol versions, but its only kosher to added the leading 
> slash for v4 mounts. Correct?  
After further review... it appears this is only a v4 issue since
v3 mounts with multiple slashes appear in /proc/mounts with the
multiple slashes (unlike v4 mounts)... 

So to restate... this normalizing of the path names (aka striping 
multiple slashes or added the leading slash) only need to occur on 
(successful) v4 mounts... Although the former would not be an 
problem with v2/v3 mounts but its not needed....

steved

Re: [PATCH] Get normalized paths for comparing NFS export paths

[Myklebust, Trond]

T24gU3VuLCAyMDEyLTAzLTA0IGF0IDE3OjU4IC0wNTAwLCBTdGV2ZSBEaWNrc29uIHdyb3RlOg0K
PiBPbiAwMy8wNC8yMDEyIDA1OjMxIFBNLCBTdGV2ZSBEaWNrc29uIHdyb3RlOg0KPiA+PiBORlN2
NCBpcyB0aGUgb25seSBwcm90b2NvbCB0aGF0IGhhcyBhIHN0YW5kYXJkIG1vdW50IHBhdGggc3lu
dGF4LCBhbmQNCj4gPj4gdGhhdCBpcyBiZWNhdXNlIHRoZSBjbGllbnQgcGVyZm9ybXMgdGhlIGpv
YiBvZiBpbnRlcnByZXRpbmcgdGhlIHBhdGgNCj4gPj4gbmFtZSBhbmQgdHJhbnNsYXRpbmcgaXQg
aW50byBQVVRST09URkggZm9sbG93ZWQgYnkgYSBidW5jaCBvZiBMT09LVVBzLg0KPiA+PiBJT1c6
IHRoZSBzdGFuZGFyZCBzeW50YXggdGhlcmUgaXMgdGhlIG9uZSBpbXBvc2VkIGJ5IHRoZSBjbGll
bnQuDQo+ID4+DQo+ID4+IFRoZXJlIGlzIG5vdGhpbmcgaW4gdGhlIE5GU3YyL3YzIE1PVU5UIHNw
ZWMgdGhhdCBzdGF0ZXMgdGhhdCBhIHBhdGgNCj4gPj4gbmVlZHMgdG8gc3RhcnQgd2l0aCAnLycu
IE5vciBpcyB0aGVyZSBldmVuIGFueXRoaW5nIGluIHRoZSBzcGVjIHRoYXQNCj4gPj4gc3RhdGVz
IHRoYXQgJy8nIGlzIHJlcXVpcmVkIHRvIGJlIHVzZWQgYXMgdGhlIGRpcmVjdG9yeSBjb21wb25l
bnQNCj4gPj4gc2VwYXJhdG9yLiBUaGUgWC9PUEVOIGRvY3Mgc3RhdGUgdGhhdCAnLycgaXMgcmVj
b21tZW5kZWQgZm9yDQo+ID4+IHBvcnRhYmlsaXR5LCBidXQgZG8gbm90IG1ha2UgaXQgYSByZXF1
aXJlbWVudC4gU2VlDQo+ID4+IGh0dHA6Ly9wdWJzLm9wZW5ncm91cC5vcmcvb25saW5lcHVicy85
NjI5Nzk5L2NoYXA4Lmh0bSN0YWdjamhfMDlfMDJfMDJfMDMgDQo+ID4+DQo+ID4+IElPVzogSSdt
IHBlcmZlY3RseSBhbGxvd2VkIHRvIHNldCB1cCBhICdtb3VudGQnIHNlcnZlciB0aGF0IHVzZXMg
J1wnIG9yDQo+ID4+IGV2ZW4gc29tZXRoaW5nIGxpa2UgJ3wnIGFzIGEgcGF0aCBjb21wb25lbnQg
c2VwYXJhdG9yLiBUaGlzIGtpbmQgb2YNCj4gPj4gcGF0Y2ggd291bGQgYnJlYWsgdGhlIGNsaWVu
dCdzIGV4aXN0aW5nIGFiaWxpdHkgdG8gbW91bnQgZnJvbSBzdWNoIGENCj4gPj4gc2VydmVyLg0K
PiA+IEFuZCB3aGVyZSBkb2VzIGFuIHNlcnZlciBsaWtlIHRoaXMgZXhpc3Q/IE9uZSB0aGF0IHVz
ZXMgJ3wnIGFzIGl0cw0KPiA+IHBhdGggY29tcG9uZW50IHNlcGFyYXRvcj8/IDstKQ0KPiA+IA0K
PiA+IEp1c3QgdG8gYmUgY2xlYXIsIHlvdSBhcmUgb2sgd2l0aCBzdHJpcGluZyB0aGUgbXVsdGlw
bGUgc2xhc2hlcywgZm9yDQo+ID4gYWxsIHByb3RvY29sIHZlcnNpb25zLCBidXQgaXRzIG9ubHkg
a29zaGVyIHRvIGFkZGVkIHRoZSBsZWFkaW5nIA0KPiA+IHNsYXNoIGZvciB2NCBtb3VudHMuIENv
cnJlY3Q/ICANCj4gQWZ0ZXIgZnVydGhlciByZXZpZXcuLi4gaXQgYXBwZWFycyB0aGlzIGlzIG9u
bHkgYSB2NCBpc3N1ZSBzaW5jZQ0KPiB2MyBtb3VudHMgd2l0aCBtdWx0aXBsZSBzbGFzaGVzIGFw
cGVhciBpbiAvcHJvYy9tb3VudHMgd2l0aCB0aGUNCj4gbXVsdGlwbGUgc2xhc2hlcyAodW5saWtl
IHY0IG1vdW50cykuLi4gDQo+IA0KPiBTbyB0byByZXN0YXRlLi4uIHRoaXMgbm9ybWFsaXppbmcg
b2YgdGhlIHBhdGggbmFtZXMgKGFrYSBzdHJpcGluZyANCj4gbXVsdGlwbGUgc2xhc2hlcyBvciBh
ZGRlZCB0aGUgbGVhZGluZyBzbGFzaCkgb25seSBuZWVkIHRvIG9jY3VyIG9uIA0KPiAoc3VjY2Vz
c2Z1bCkgdjQgbW91bnRzLi4uIEFsdGhvdWdoIHRoZSBmb3JtZXIgd291bGQgbm90IGJlIGFuIA0K
PiBwcm9ibGVtIHdpdGggdjIvdjMgbW91bnRzIGJ1dCBpdHMgbm90IG5lZWRlZC4uLi4NCg0KTm9y
bWFsaXNpbmcgZm9yIHY0IG9ubHkgaXMgT0sgYXMgZmFyIGFzIEknbSBjb25jZXJuZWQsIGJ1dCBh
cyBhbg0KYWx0ZXJuYXRpdmU6IGlzIHRoZXJlIGFueSByZWFzb24gd2h5IHlvdSBjYW4ndCBqdXN0
IGdyYWIgdGhlIHBhdGggZnJvbQ0KJy9wcm9jL21vdW50cyc/DQoNCi0tIA0KVHJvbmQgTXlrbGVi
dXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXINCg0KTmV0QXBwDQpUcm9uZC5NeWtsZWJ1
c3RAbmV0YXBwLmNvbQ0Kd3d3Lm5ldGFwcC5jb20NCg0K

Re: [PATCH] Get normalized paths for comparing NFS export paths

[Steve Dickson]

On 03/04/2012 06:26 PM, Myklebust, Trond wrote:
> On Sun, 2012-03-04 at 17:58 -0500, Steve Dickson wrote:
>> So to restate... this normalizing of the path names (aka striping 
>> multiple slashes or added the leading slash) only need to occur on 
>> (successful) v4 mounts... Although the former would not be an 
>> problem with v2/v3 mounts but its not needed....
> 
> Normalising for v4 only is OK as far as I'm concerned, but as an
> alternative: is there any reason why you can't just grab the path from
> '/proc/mounts'?
> 
Well in some distros /etc/mtab is symbolically linked to /proc/mounts
so this problem does not exist. But from what I've been told not
all distros do that. Plus being backward compatible with older 
release of distro is a good thing... IMHO...

steved.
  

Re: [PATCH] Get normalized paths for comparing NFS export paths

[Myklebust, Trond]

T24gU3VuLCAyMDEyLTAzLTA0IGF0IDE5OjAzIC0wNTAwLCBTdGV2ZSBEaWNrc29uIHdyb3RlOg0K
PiANCj4gT24gMDMvMDQvMjAxMiAwNjoyNiBQTSwgTXlrbGVidXN0LCBUcm9uZCB3cm90ZToNCj4g
PiBPbiBTdW4sIDIwMTItMDMtMDQgYXQgMTc6NTggLTA1MDAsIFN0ZXZlIERpY2tzb24gd3JvdGU6
DQo+ID4+IFNvIHRvIHJlc3RhdGUuLi4gdGhpcyBub3JtYWxpemluZyBvZiB0aGUgcGF0aCBuYW1l
cyAoYWthIHN0cmlwaW5nIA0KPiA+PiBtdWx0aXBsZSBzbGFzaGVzIG9yIGFkZGVkIHRoZSBsZWFk
aW5nIHNsYXNoKSBvbmx5IG5lZWQgdG8gb2NjdXIgb24gDQo+ID4+IChzdWNjZXNzZnVsKSB2NCBt
b3VudHMuLi4gQWx0aG91Z2ggdGhlIGZvcm1lciB3b3VsZCBub3QgYmUgYW4gDQo+ID4+IHByb2Js
ZW0gd2l0aCB2Mi92MyBtb3VudHMgYnV0IGl0cyBub3QgbmVlZGVkLi4uLg0KPiA+IA0KPiA+IE5v
cm1hbGlzaW5nIGZvciB2NCBvbmx5IGlzIE9LIGFzIGZhciBhcyBJJ20gY29uY2VybmVkLCBidXQg
YXMgYW4NCj4gPiBhbHRlcm5hdGl2ZTogaXMgdGhlcmUgYW55IHJlYXNvbiB3aHkgeW91IGNhbid0
IGp1c3QgZ3JhYiB0aGUgcGF0aCBmcm9tDQo+ID4gJy9wcm9jL21vdW50cyc/DQo+ID4gDQo+IFdl
bGwgaW4gc29tZSBkaXN0cm9zIC9ldGMvbXRhYiBpcyBzeW1ib2xpY2FsbHkgbGlua2VkIHRvIC9w
cm9jL21vdW50cw0KPiBzbyB0aGlzIHByb2JsZW0gZG9lcyBub3QgZXhpc3QuIEJ1dCBmcm9tIHdo
YXQgSSd2ZSBiZWVuIHRvbGQgbm90DQo+IGFsbCBkaXN0cm9zIGRvIHRoYXQuIFBsdXMgYmVpbmcg
YmFja3dhcmQgY29tcGF0aWJsZSB3aXRoIG9sZGVyIA0KPiByZWxlYXNlIG9mIGRpc3RybyBpcyBh
IGdvb2QgdGhpbmcuLi4gSU1ITy4uLg0KDQpXaGljaCBpcyB3aHkgSSdtIGFza2luZyB5b3UgaWYg
eW91IGNhbid0IGNvcHkgZnJvbSAvcHJvYy9tb3VudHMuIERpc3Ryb3MNCndoaWNoIGFscmVhZHkg
aGF2ZSBhIHN5bWxpbmsgYmV0d2VlayAvZXRjL210YWIgYW5kIC9wcm9jL21vdW50cyBkb24ndA0K
aGF2ZSBhbnkgb2YgdGhlc2UgcHJvYmxlbXMuLi4NCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxp
bnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVzdEBuZXRh
cHAuY29tDQp3d3cubmV0YXBwLmNvbQ0KDQo=

Re: [PATCH] Get normalized paths for comparing NFS export paths

[Malahal Naineni]

Myklebust, Trond [Trond.Myklebust@netapp.com] wrote:
> On Sun, 2012-03-04 at 19:03 -0500, Steve Dickson wrote:
> > 
> > On 03/04/2012 06:26 PM, Myklebust, Trond wrote:
> > > On Sun, 2012-03-04 at 17:58 -0500, Steve Dickson wrote:
> > >> So to restate... this normalizing of the path names (aka striping 
> > >> multiple slashes or added the leading slash) only need to occur on 
> > >> (successful) v4 mounts... Although the former would not be an 
> > >> problem with v2/v3 mounts but its not needed....
> > > 
> > > Normalising for v4 only is OK as far as I'm concerned, but as an
> > > alternative: is there any reason why you can't just grab the path from
> > > '/proc/mounts'?

I don't know where it grabs the path (mtab vs proc file), but it
probably doesn't matter. Either one should work for umount. AFAIR, it is
only the matching entries issue as the umount needs to delete the
correct entry from matb.  In other words, nfs umount bails out without
calling umount when there is no matching entry.

Thanks, Malahal.

Re: [PATCH] Get normalized paths for comparing NFS export paths

[Steve Dickson]

On 03/04/2012 09:04 PM, Myklebust, Trond wrote:
> On Sun, 2012-03-04 at 19:03 -0500, Steve Dickson wrote:
>>
>> On 03/04/2012 06:26 PM, Myklebust, Trond wrote:
>>> On Sun, 2012-03-04 at 17:58 -0500, Steve Dickson wrote:
>>>> So to restate... this normalizing of the path names (aka striping 
>>>> multiple slashes or added the leading slash) only need to occur on 
>>>> (successful) v4 mounts... Although the former would not be an 
>>>> problem with v2/v3 mounts but its not needed....
>>>
>>> Normalising for v4 only is OK as far as I'm concerned, but as an
>>> alternative: is there any reason why you can't just grab the path from
>>> '/proc/mounts'?
>>>
>> Well in some distros /etc/mtab is symbolically linked to /proc/mounts
>> so this problem does not exist. But from what I've been told not
>> all distros do that. Plus being backward compatible with older 
>> release of distro is a good thing... IMHO...
> 
> Which is why I'm asking you if you can't copy from /proc/mounts. Distros
> which already have a symlink betweek /etc/mtab and /proc/mounts don't
> have any of these problems...
> 
Personally I think it simpler to just normalize the paths before 
writing it to the mtab verses open up and searching /proc/mounts
esp on client where there is large amount of mounts...  

steved.

Re: [PATCH] Get normalized paths for comparing NFS export paths

[Malahal Naineni]

Steve Dickson [SteveD@redhat.com] wrote:
> 
> > Which is why I'm asking you if you can't copy from /proc/mounts. Distros
> > which already have a symlink betweek /etc/mtab and /proc/mounts don't
> > have any of these problems...
> > 
> Personally I think it simpler to just normalize the paths before 
> writing it to the mtab verses open up and searching /proc/mounts
> esp on client where there is large amount of mounts...  

We always open up and search /proc/mounts today for the matching
mountpoint to know the exact version of nfs, right? The umount time
normalization is going to cost us? Or you proposing a patch that is
going to avoid opening up /proc/mounts altogether?

Regards, Malahal.

Re: [PATCH] Get normalized paths for comparing NFS export paths

[Steve Dickson]

On 03/05/2012 09:47 AM, Malahal Naineni wrote:
> Steve Dickson [SteveD@redhat.com] wrote:
>>
>>> Which is why I'm asking you if you can't copy from /proc/mounts. Distros
>>> which already have a symlink betweek /etc/mtab and /proc/mounts don't
>>> have any of these problems...
>>>
>> Personally I think it simpler to just normalize the paths before 
>> writing it to the mtab verses open up and searching /proc/mounts
>> esp on client where there is large amount of mounts...  
> 
> We always open up and search /proc/mounts today for the matching
> mountpoint to know the exact version of nfs, right? 
Yes, we open up /proc/mount to get the authoritative version.

> The umount time normalization is going to cost us? 
Nothing... but I thought it might be easier to do the 
normalization after a successful v4 iff the the mtab
is writable.
 
> Or you proposing a patch that is going to avoid opening up /proc/mounts altogether?
No... umount has to open /proc/mounts...

steved.