nfs-utils: Something is wrong in is_vers4()

nfs-utils: Something is wrong in is_vers4()

[NeilBrown]

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


in nfs-utils, in utils/mount/mount_libmount.c there is a function

 is_vers4()

which
 /* returns: error = -1, success = 0 , unknown = 1 */

which seems odd to me... I would have chosen '1' for success (it is vers 4),
0 for failure (not vers 4), and maybe -1 for error (something went wrong).

This is used as follows:
			switch (is_vers4(cxt)) {
			case 0:
				/* We ignore the error from nfs_umount23.
				 * If the actual umount succeeds (in del_mtab),
				 * we don't want to signal an error, as that
				 * could cause /sbin/mount to retry!
				 */
				nfs_umount23(mnt_context_get_source(cxt), opts);
				break;
			case 1:			/* unknown */
				break;
			default:		/* error */
				goto err;
			}

so in the '0' (success, it is vers4) case we do nfs_umount23.  Odd.
In the '1' (unknown, so presumably vers2 or 3) we don't.  Ever.  Very odd.

So we don't currently send MOUNT_UMNT requests for v2 or v3.
We don't for v4 either because nfs_umount_do_umnt contains:

	/* Skip UMNT call for vers=4 mounts */
	if (nfs_pmap.pm_vers == 4)
		return EX_SUCCESS;

so maybe is_vers4 isn't needed?

Looks like something needs to be fixed here but I'm not entirely sure what.
Karel??

BTW Steve, Karel's "[PATCH] umount.nfs: ignore non-nfs filesystems"
which appears in email:

From: Karel Zak <kzak@redhat.com>
To: NeilBrown <neilb@suse.de>
Cc: Steve Dickson <SteveD@redhat.com>, NFS <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] umount.nfs: restore correct error status when umount fails.
Date: Thu, 12 Jul 2012 18:44:20 +0200

hasn't been applied, but probably should be.

Thanks,
NeilBrown


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

Re: nfs-utils: Something is wrong in is_vers4()

[Karel Zak]

On Tue, Jul 31, 2012 at 04:58:42PM +1000, NeilBrown wrote:
> 
> in nfs-utils, in utils/mount/mount_libmount.c there is a function
> 
>  is_vers4()
> 
> which
>  /* returns: error = -1, success = 0 , unknown = 1 */
> 
> which seems odd to me... I would have chosen '1' for success (it is vers 4),
> 0 for failure (not vers 4), and maybe -1 for error (something went wrong).
> 
> This is used as follows:
> 			switch (is_vers4(cxt)) {
> 			case 0:
> 				/* We ignore the error from nfs_umount23.
> 				 * If the actual umount succeeds (in del_mtab),
> 				 * we don't want to signal an error, as that
> 				 * could cause /sbin/mount to retry!
> 				 */
> 				nfs_umount23(mnt_context_get_source(cxt), opts);
> 				break;
> 			case 1:			/* unknown */
> 				break;
> 			default:		/* error */
> 				goto err;
> 			}

 The same code you can found in original non-libmount version
 in nfsumount.c *but* with correct nfs_umount_is_vers4() return codes.

> 
> so in the '0' (success, it is vers4) case we do nfs_umount23.  Odd.
> In the '1' (unknown, so presumably vers2 or 3) we don't.  Ever.  Very odd.

You're right, should be (copy & past from nfsumount.c):

   * Returns 1 if "mc" is an NFSv4 mount, zero if not, and
   * -1 if some error occurred.

...probably my bug. Sorry.

> So we don't currently send MOUNT_UMNT requests for v2 or v3.
> We don't for v4 either because nfs_umount_do_umnt contains:
> 
> 	/* Skip UMNT call for vers=4 mounts */
> 	if (nfs_pmap.pm_vers == 4)
> 		return EX_SUCCESS;
> 
> so maybe is_vers4 isn't needed?
> 
> Looks like something needs to be fixed here but I'm not entirely sure what.
> Karel??

The is_vers4() should be fixed to be compatible with the original code.

> BTW Steve, Karel's "[PATCH] umount.nfs: ignore non-nfs filesystems"
> which appears in email:
> 
> From: Karel Zak <kzak@redhat.com>
> To: NeilBrown <neilb@suse.de>
> Cc: Steve Dickson <SteveD@redhat.com>, NFS <linux-nfs@vger.kernel.org>
> Subject: Re: [PATCH] umount.nfs: restore correct error status when umount fails.
> Date: Thu, 12 Jul 2012 18:44:20 +0200
> 
> hasn't been applied, but probably should be.

 Yes.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: nfs-utils: Something is wrong in is_vers4()

[Steve Dickson]

On 07/31/2012 02:58 AM, NeilBrown wrote:
> 
> in nfs-utils, in utils/mount/mount_libmount.c there is a function
> 
>  is_vers4()
> 
> which
>  /* returns: error = -1, success = 0 , unknown = 1 */
> 
> which seems odd to me... I would have chosen '1' for success (it is vers 4),
> 0 for failure (not vers 4), and maybe -1 for error (something went wrong).
> 
> This is used as follows:
> 			switch (is_vers4(cxt)) {
> 			case 0:
> 				/* We ignore the error from nfs_umount23.
> 				 * If the actual umount succeeds (in del_mtab),
> 				 * we don't want to signal an error, as that
> 				 * could cause /sbin/mount to retry!
> 				 */
> 				nfs_umount23(mnt_context_get_source(cxt), opts);
> 				break;
> 			case 1:			/* unknown */
> 				break;
> 			default:		/* error */
> 				goto err;
> 			}
> 
> so in the '0' (success, it is vers4) case we do nfs_umount23.  Odd.
> In the '1' (unknown, so presumably vers2 or 3) we don't.  Ever.  Very odd.
> 
> So we don't currently send MOUNT_UMNT requests for v2 or v3.
> We don't for v4 either because nfs_umount_do_umnt contains:
> 
> 	/* Skip UMNT call for vers=4 mounts */
> 	if (nfs_pmap.pm_vers == 4)
> 		return EX_SUCCESS;
> 
> so maybe is_vers4 isn't needed?
> 
> Looks like something needs to be fixed here but I'm not entirely sure what.
> Karel??
> 
> BTW Steve, Karel's "[PATCH] umount.nfs: ignore non-nfs filesystems"
> which appears in email:
> 
> From: Karel Zak <kzak@redhat.com>
> To: NeilBrown <neilb@suse.de>
> Cc: Steve Dickson <SteveD@redhat.com>, NFS <linux-nfs@vger.kernel.org>
> Subject: Re: [PATCH] umount.nfs: restore correct error status when umount fails.
> Date: Thu, 12 Jul 2012 18:44:20 +0200
> 
> hasn't been applied, but probably should be.
I beg to differ....

commit 76908c3f14a12e865054ea5d6e4cad201c28839a
Author: NeilBrown <neilb@suse.de>
Date:   Mon Jul 16 08:43:28 2012 -0400

    mount.nfs: restore correct error status when umount fails

Or am I missing something?

steved.

Re: nfs-utils: Something is wrong in is_vers4()

[Karel Zak]

On Tue, Jul 31, 2012 at 01:04:58PM -0400, Steve Dickson wrote:
> > BTW Steve, Karel's "[PATCH] umount.nfs: ignore non-nfs filesystems"
> > which appears in email:
> > 
> > From: Karel Zak <kzak@redhat.com>
> > To: NeilBrown <neilb@suse.de>
> > Cc: Steve Dickson <SteveD@redhat.com>, NFS <linux-nfs@vger.kernel.org>
> > Subject: Re: [PATCH] umount.nfs: restore correct error status when umount fails.
> > Date: Thu, 12 Jul 2012 18:44:20 +0200
> > 
> > hasn't been applied, but probably should be.
> I beg to differ....
> 
> commit 76908c3f14a12e865054ea5d6e4cad201c28839a
> Author: NeilBrown <neilb@suse.de>
> Date:   Mon Jul 16 08:43:28 2012 -0400
> 
>     mount.nfs: restore correct error status when umount fails
> 
> Or am I missing something?

 Yes, Neil found two problems:

    http://www.spinics.net/lists/linux-nfs/msg31466.html

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: nfs-utils: Something is wrong in is_vers4()

[Steve Dickson]

On 07/31/2012 02:37 PM, Karel Zak wrote:
> On Tue, Jul 31, 2012 at 01:04:58PM -0400, Steve Dickson wrote:
>>> BTW Steve, Karel's "[PATCH] umount.nfs: ignore non-nfs filesystems"
>>> which appears in email:
>>>
>>> From: Karel Zak <kzak@redhat.com>
>>> To: NeilBrown <neilb@suse.de>
>>> Cc: Steve Dickson <SteveD@redhat.com>, NFS <linux-nfs@vger.kernel.org>
>>> Subject: Re: [PATCH] umount.nfs: restore correct error status when umount fails.
>>> Date: Thu, 12 Jul 2012 18:44:20 +0200
>>>
>>> hasn't been applied, but probably should be.
>> I beg to differ....
>>
>> commit 76908c3f14a12e865054ea5d6e4cad201c28839a
>> Author: NeilBrown <neilb@suse.de>
>> Date:   Mon Jul 16 08:43:28 2012 -0400
>>
>>     mount.nfs: restore correct error status when umount fails
>>
>> Or am I missing something?
> 
>  Yes, Neil found two problems:
> 
>     http://www.spinics.net/lists/linux-nfs/msg31466.html
Ah... Found it and committed it... 

steved.

Re: nfs-utils: Something is wrong in is_vers4()

[NeilBrown]

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

On Wed, 01 Aug 2012 11:22:04 -0400 Steve Dickson <SteveD@redhat.com> wrote:

> 
> 
> On 07/31/2012 02:37 PM, Karel Zak wrote:
> > On Tue, Jul 31, 2012 at 01:04:58PM -0400, Steve Dickson wrote:
> >>> BTW Steve, Karel's "[PATCH] umount.nfs: ignore non-nfs filesystems"
> >>> which appears in email:
> >>>
> >>> From: Karel Zak <kzak@redhat.com>
> >>> To: NeilBrown <neilb@suse.de>
> >>> Cc: Steve Dickson <SteveD@redhat.com>, NFS <linux-nfs@vger.kernel.org>
> >>> Subject: Re: [PATCH] umount.nfs: restore correct error status when umount fails.
> >>> Date: Thu, 12 Jul 2012 18:44:20 +0200
> >>>
> >>> hasn't been applied, but probably should be.
> >> I beg to differ....
> >>
> >> commit 76908c3f14a12e865054ea5d6e4cad201c28839a
> >> Author: NeilBrown <neilb@suse.de>
> >> Date:   Mon Jul 16 08:43:28 2012 -0400
> >>
> >>     mount.nfs: restore correct error status when umount fails
> >>
> >> Or am I missing something?
> > 
> >  Yes, Neil found two problems:
> > 
> >     http://www.spinics.net/lists/linux-nfs/msg31466.html
> Ah... Found it and committed it... 
> 
> steved.

Thanks.
So here is one more :-)  This  fixes the the bug in $SUBJECT

NeilBrown

From: Neil Brown <neilb@suse.de>
Date: Thu, 2 Aug 2012 10:20:13 +1000
Subject: [PATCH] umount: use correct return value for is_vers4.

is_vers4 in mount_libmount.c is based on nfs_umount_is_vers4
in nfsumount.c, except the return values are reversed.
The result of this is:
 - a MOUNT_UMNT call is not sent when an NFSv3 or NFSv2
   filesystem is unmounted
 - a MOUNT_UMNT call *is* sent when and 'nfs4' filesystem
   is unmounted (but not when an 'nfs -o vers=4 filesystem
   is unmounted, as that is checked elsewhere).

Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/utils/mount/mount_libmount.c b/utils/mount/mount_libmount.c
index ddf61b2..701d41e 100644
--- a/utils/mount/mount_libmount.c
+++ b/utils/mount/mount_libmount.c
@@ -140,14 +140,14 @@ static int try_mount(struct libmnt_context *cxt, int bg)
 	return ret;
 }
 
-/* returns: error = -1, success = 0 , unknown = 1 */
+/* returns: error = -1, success = 1 , not vers4 == 0 */
 static int is_vers4(struct libmnt_context *cxt)
 {
 	struct libmnt_fs *fs = mnt_context_get_fs(cxt);
 	struct libmnt_table *tb = NULL;
 	const char *src = mnt_context_get_source(cxt),
 		   *tgt = mnt_context_get_target(cxt);
-	int rc = 1;
+	int rc = 0;
 
 	if (!src || !tgt)
 		return -1;
@@ -163,7 +163,7 @@ static int is_vers4(struct libmnt_context *cxt)
 	if (fs) {
 		const char *type = mnt_fs_get_fstype(fs);
 		if (type && strcmp(type, "nfs4") == 0)
-			rc = 0;
+			rc = 1;
 	}
 	mnt_free_table(tb);
 	return rc;

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

Re: nfs-utils: Something is wrong in is_vers4()

[Steve Dickson]

On 08/01/2012 08:21 PM, NeilBrown wrote:
> is_vers4 in mount_libmount.c is based on nfs_umount_is_vers4
> in nfsumount.c, except the return values are reversed.
> The result of this is:
>  - a MOUNT_UMNT call is not sent when an NFSv3 or NFSv2
>    filesystem is unmounted
>  - a MOUNT_UMNT call *is* sent when and 'nfs4' filesystem
>    is unmounted (but not when an 'nfs -o vers=4 filesystem
>    is unmounted, as that is checked elsewhere).
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
Committed...

steved.

> 
> diff --git a/utils/mount/mount_libmount.c b/utils/mount/mount_libmount.c
> index ddf61b2..701d41e 100644
> --- a/utils/mount/mount_libmount.c
> +++ b/utils/mount/mount_libmount.c
> @@ -140,14 +140,14 @@ static int try_mount(struct libmnt_context *cxt, int bg)
>  	return ret;
>  }
>  
> -/* returns: error = -1, success = 0 , unknown = 1 */
> +/* returns: error = -1, success = 1 , not vers4 == 0 */
>  static int is_vers4(struct libmnt_context *cxt)
>  {
>  	struct libmnt_fs *fs = mnt_context_get_fs(cxt);
>  	struct libmnt_table *tb = NULL;
>  	const char *src = mnt_context_get_source(cxt),
>  		   *tgt = mnt_context_get_target(cxt);
> -	int rc = 1;
> +	int rc = 0;
>  
>  	if (!src || !tgt)
>  		return -1;
> @@ -163,7 +163,7 @@ static int is_vers4(struct libmnt_context *cxt)
>  	if (fs) {
>  		const char *type = mnt_fs_get_fstype(fs);
>  		if (type && strcmp(type, "nfs4") == 0)
> -			rc = 0;
> +			rc = 1;
>  	}
>  	mnt_free_table(tb);
>  	return rc;