retry= is additive with the text-based mount interface

retry= is additive with the text-based mount interface

[Steinar H. Gunderson]

Hi,

(Via a user bug report)

It seems retry= is now additive with the text-based mount interface. In
particular, "mount -o retry=0" still gives a two-minute timeout. Is this
intentional?

/* Steinar */
-- 
Homepage: http://www.sesse.net/

Re: retry= is additive with the text-based mount interface

[Chuck Lever]

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

On Apr 25, 2008, at 4:05 AM, Steinar H. Gunderson wrote:
> (Via a user bug report)
>
> It seems retry= is now additive with the text-based mount interface.  
> In
> particular, "mount -o retry=0" still gives a two-minute timeout. Is  
> this
> intentional?


No, it's broken.  Can you test the attached patch?


[-- Attachment #2: fix-retry-option --]
[-- Type: application/octet-stream, Size: 2960 bytes --]

text-based mount command: Fix retry= option

Steinar Gunderson reports:

"It seems retry= is now additive with the text-based mount interface. In
particular, "mount -o retry=0" still gives a two-minute timeout."

Make retry option parsing more robust, while we're at it.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 utils/mount/stropts.c |   55 ++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index cadb1f4..4df9ad9 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -65,6 +65,14 @@
 #define NFS_MAXPATHNAME		(1024)
 #endif
 
+#ifndef NFS_DEF_FG_TIMEOUT_MINUTES
+#define NFS_DEF_FG_TIMEOUT_MINUTES	(2ul)
+#endif
+
+#ifndef NFS_DEF_BG_TIMEOUT_MINUTES
+#define NFS_DEF_BG_TIMEOUT_MINUTES	(10000ul)
+#endif
+
 extern int nfs_mount_data_version;
 extern char *progname;
 extern int verbose;
@@ -141,6 +149,35 @@ static int fill_ipv4_sockaddr(const char *hostname, struct sockaddr_in *addr)
 }
 
 /*
+ * Set up a timeout value based on the value of the "retry=" option.
+ *
+ * Returns 1 if the option was parsed successfully, otherwise zero.
+ * Returns the parsed timeout, or the default, in *timeout.
+ */
+static int parse_retry_option(time_t *timeout, struct mount_options *options,
+			      unsigned long timeout_minutes)
+{
+	char *retry_option, *endptr;
+
+	retry_option = po_get(options, "retry");
+	if (retry_option) {
+		long tmp;
+
+		errno = 0;
+		tmp = strtol(retry_option, &endptr, 10);
+		if (errno || endptr == retry_option || tmp < 0) {
+			nfs_error(_("%s: incorrect retry timeout specified"),
+					progname);
+			return 0;
+		}
+		timeout_minutes = tmp;
+	}
+
+	*timeout += time(NULL) + (time_t)(timeout_minutes * 60);
+	return 1;
+}
+
+/*
  * Append the 'addr=' option to the options string to pass a resolved
  * server address to the kernel.  After a successful mount, this address
  * is also added to /etc/mtab for use when unmounting.
@@ -535,13 +572,10 @@ static int nfsmount_fg(const char *spec, const char *node,
 		       char **extra_opts)
 {
 	unsigned int secs = 1;
-	time_t timeout = time(NULL);
-	char *retry;
+	time_t timeout;
 
-	timeout += 60 * 2;		/* default: 2 minutes */
-	retry = po_get(options, "retry");
-	if (retry)
-		timeout += 60 * atoi(retry);
+	if (!parse_retry_option(&timeout, options, NFS_DEF_FG_TIMEOUT_MINUTES))
+		return EX_FAIL;
 
 	if (verbose)
 		printf(_("%s: timeout set for %s"),
@@ -612,13 +646,10 @@ static int nfsmount_child(const char *spec, const char *node,
 			  int fake, char **extra_opts)
 {
 	unsigned int secs = 1;
-	time_t timeout = time(NULL);
-	char *retry;
+	time_t timeout;
 
-	timeout += 60 * 10000;		/* default: 10,000 minutes */
-	retry = po_get(options, "retry");
-	if (retry)
-		timeout += 60 * atoi(retry);
+	if (!parse_retry_option(&timeout, options, NFS_DEF_BG_TIMEOUT_MINUTES))
+		return EX_FAIL;
 
 	for (;;) {
 		if (sleep(secs))

[-- Attachment #3: Type: text/plain, Size: 50 bytes --]



--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: retry= is additive with the text-based mount interface

[Steinar H. Gunderson]

On Fri, Apr 25, 2008 at 12:20:19PM -0400, Chuck Lever wrote:
>> It seems retry= is now additive with the text-based mount interface.  In
>> particular, "mount -o retry=0" still gives a two-minute timeout. Is  this
>> intentional?
> No, it's broken.  Can you test the attached patch?

It seems it creates rather odd behavior:


fugl:~# mount -t nfs -v -o retry=0 10.0.0.11:/foo /mnt
mount.nfs: timeout set for Thu Jan  1 14:40:25 1970
mount.nfs: text-based options: 'retry=0,addr=10.0.0.11'

fugl:~# mount -t nfs -v -o retry=0 10.0.0.11:/foo /mnt
mount.nfs: timeout set for Sat Jan 10 17:07:36 1970
mount.nfs: text-based options: 'retry=0,addr=10.0.0.11'

/* Steinar */
-- 
Homepage: http://www.sesse.net/

Re: retry= is additive with the text-based mount interface

[Chuck Lever]

On Apr 25, 2008, at 12:34 PM, Steinar H. Gunderson wrote:
> On Fri, Apr 25, 2008 at 12:20:19PM -0400, Chuck Lever wrote:
>>> It seems retry= is now additive with the text-based mount  
>>> interface.  In
>>> particular, "mount -o retry=0" still gives a two-minute timeout.  
>>> Is  this
>>> intentional?
>> No, it's broken.  Can you test the attached patch?
>
> It seems it creates rather odd behavior:
>
>
> fugl:~# mount -t nfs -v -o retry=0 10.0.0.11:/foo /mnt
> mount.nfs: timeout set for Thu Jan  1 14:40:25 1970
> mount.nfs: text-based options: 'retry=0,addr=10.0.0.11'

On my test system (Fedora 7) I get correct behavior with both /bin/ 
mount and when using /sbin/mount.nfs directly.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: retry= is additive with the text-based mount interface

[Steinar H. Gunderson]

On Fri, Apr 25, 2008 at 12:45:36PM -0400, Chuck Lever wrote:
>> fugl:~# mount -t nfs -v -o retry=0 10.0.0.11:/foo /mnt
>> mount.nfs: timeout set for Thu Jan  1 14:40:25 1970
>> mount.nfs: text-based options: 'retry=0,addr=10.0.0.11'
> On my test system (Fedora 7) I get correct behavior with both /bin/mount 
> and when using /sbin/mount.nfs directly.

It seems the patch has an issue:

+       if (!parse_retry_option(&timeout, options, NFS_DEF_FG_TIMEOUT_MINUTES))

where parse_retry_option() _adds_ to timeout. But timeout is not initialized :-)

/* Steinar */
-- 
Homepage: http://www.sesse.net/

Re: retry= is additive with the text-based mount interface

[Trond Myklebust]

On Fri, 2008-04-25 at 18:59 +0200, Steinar H. Gunderson wrote:
> On Fri, Apr 25, 2008 at 12:45:36PM -0400, Chuck Lever wrote:
> >> fugl:~# mount -t nfs -v -o retry=0 10.0.0.11:/foo /mnt
> >> mount.nfs: timeout set for Thu Jan  1 14:40:25 1970
> >> mount.nfs: text-based options: 'retry=0,addr=10.0.0.11'
> > On my test system (Fedora 7) I get correct behavior with both /bin/mount 
> > and when using /sbin/mount.nfs directly.
> 
> It seems the patch has an issue:
> 
> +       if (!parse_retry_option(&timeout, options, NFS_DEF_FG_TIMEOUT_MINUTES))
> 
> where parse_retry_option() _adds_ to timeout. But timeout is not initialized :-)

Why would we ever want to do anything other than just set the timeout
here, Chuck?

Trond

Re: retry= is additive with the text-based mount interface

[Chuck Lever]

On Apr 25, 2008, at 12:59 PM, Steinar H. Gunderson wrote:
> On Fri, Apr 25, 2008 at 12:45:36PM -0400, Chuck Lever wrote:
>>> fugl:~# mount -t nfs -v -o retry=0 10.0.0.11:/foo /mnt
>>> mount.nfs: timeout set for Thu Jan  1 14:40:25 1970
>>> mount.nfs: text-based options: 'retry=0,addr=10.0.0.11'
>> On my test system (Fedora 7) I get correct behavior with both /bin/ 
>> mount
>> and when using /sbin/mount.nfs directly.
>
> It seems the patch has an issue:
>
> +       if (!parse_retry_option(&timeout, options,  
> NFS_DEF_FG_TIMEOUT_MINUTES))
>
> where parse_retry_option() _adds_ to timeout. But timeout is not  
> initialized :-)

Just change "*timeout +=" to "*timeout =" in parse_retry_option().

That was the bug I was trying to fix in the first place.  Sigh.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: retry= is additive with the text-based mount interface

[Steinar H. Gunderson]

On Fri, Apr 25, 2008 at 01:11:02PM -0400, Chuck Lever wrote:
> Just change "*timeout +=" to "*timeout =" in parse_retry_option().
>
> That was the bug I was trying to fix in the first place.  Sigh.

That fixes it -- thanks!

/* Steinar */
-- 
Homepage: http://www.sesse.net/

Re: retry= is additive with the text-based mount interface

[Chuck Lever]

On Apr 25, 2008, at 1:38 PM, Steinar H. Gunderson wrote:
> On Fri, Apr 25, 2008 at 01:11:02PM -0400, Chuck Lever wrote:
>> Just change "*timeout +=" to "*timeout =" in parse_retry_option().
>>
>> That was the bug I was trying to fix in the first place.  Sigh.
>
> That fixes it -- thanks!

Thanks for testing.  I'll post a corrected patch to Mr. Dickson in a  
bit.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com