[PATCH 0/3] IPv6-related nfs-utils bugs and regressions

[PATCH 0/3] IPv6-related nfs-utils bugs and regressions

[Chuck Lever]

Two of these are build regressions: the new code uses macros not
defined in glibc 2.2.5 headers.  The third fixes a longstanding bug
with how statd and sm-notify sweep the sm/ and sm.bak/ directories
looking for monitored host records.

---

Chuck Lever (3):
      sm-notify: Make use of AI_NUMERICSERV conditional
      libnsm.a: Replace __attribute_noinline__
      libnsm.a: sm-notify sometimes ignores monitored hosts


 support/nsm/file.c      |   48 ++++++++++++++++++++++++++++++++++-------------
 utils/statd/sm-notify.c |    6 ++++++
 2 files changed, 41 insertions(+), 13 deletions(-)

-- 
Chuck Lever

[PATCH 1/3] libnsm.a: sm-notify sometimes ignores monitored hosts

[Chuck Lever]

Monitored host information is stored in files under /var/lib/nfs.
When visiting entries in the monitored hosts directory, libnsm.a
examines the value of dirent.d_type to determine if an entry is a
regular file.

According to readdir(3), the d_type field is not supported by all
file system types.  My root file system happens to be one where d_type
isn't supported.  Typical installations that use an ext-derived root
file system are not exposed to this issue, but those who use xfs, for
instance, are.

On such file systems, not only are remote peers not notified of
reboots, but the NSM state number is never incremented.  A statd warm
restart would not re-monitor any hosts that were monitored before
the restart.

When writing support/nsm/file.c, I copied the use of d_type from the
original statd code, so this has likely been an issue for some time.

Replace the use of d_type in support/nsm/file.c with a call to
lstat(2).  It's extra code, but is guaranteed to work on all file
system types.

Note there is a usage of d_type in gssd.  I'll let gssd and rpcpipefs
experts decide whether that's worth changing.

Fix for:

  https://bugzilla.linux-nfs.org/show_bug.cgi?id=193

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

 support/nsm/file.c |   38 ++++++++++++++++++++++++++++++--------
 1 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/support/nsm/file.c b/support/nsm/file.c
index f4baeb9..ba2a9ce 100644
--- a/support/nsm/file.c
+++ b/support/nsm/file.c
@@ -568,11 +568,13 @@ nsm_retire_monitored_hosts(void)
 
 	while ((de = readdir(dir)) != NULL) {
 		char *src, *dst;
+		struct stat stb;
 
-		if (de->d_type != (unsigned char)DT_REG)
-			continue;
-		if (de->d_name[0] == '.')
+		if (de->d_name[0] == '.') {
+			xlog(D_GENERAL, "Skipping dot file %s",
+					de->d_name);
 			continue;
+		}
 
 		src = nsm_make_record_pathname(NSM_MONITOR_DIR, de->d_name);
 		if (src == NULL) {
@@ -580,6 +582,20 @@ nsm_retire_monitored_hosts(void)
 			continue;
 		}
 
+		/* NB: not all file systems fill in d_type correctly */
+		if (lstat(src, &stb) == -1) {
+			xlog_warn("Bad monitor file %s, skipping: %m",
+					de->d_name);
+			free(src);
+			continue;
+		}
+		if (!S_ISREG(stb.st_mode)) {
+			xlog(D_GENERAL, "Skipping non-regular file %s",
+					de->d_name);
+			free(src);
+			continue;
+		}
+
 		dst = nsm_make_record_pathname(NSM_NOTIFY_DIR, de->d_name);
 		if (dst == NULL) {
 			free(src);
@@ -846,7 +862,7 @@ nsm_read_line(const char *hostname, const time_t timestamp, char *line,
 }
 
 /*
- * Given a filename, reads data from a file under NSM_MONITOR_DIR
+ * Given a filename, reads data from a file under "directory"
  * and invokes @func so caller can populate their in-core
  * database with this data.
  */
@@ -863,10 +879,15 @@ nsm_load_host(const char *directory, const char *filename, nsm_populate_t func)
 	if (path == NULL)
 		goto out_err;
 
-	if (stat(path, &stb) == -1) {
+	if (lstat(path, &stb) == -1) {
 		xlog(L_ERROR, "Failed to stat %s: %m", path);
 		goto out_freepath;
 	}
+	if (!S_ISREG(stb.st_mode)) {
+		xlog(D_GENERAL, "Skipping non-regular file %s",
+				path);
+		goto out_freepath;
+	}
 
 	f = fopen(path, "r");
 	if (f == NULL) {
@@ -913,10 +934,11 @@ nsm_load_dir(const char *directory, nsm_populate_t func)
 	}
 
 	while ((de = readdir(dir)) != NULL) {
-		if (de->d_type != (unsigned char)DT_REG)
-			continue;
-		if (de->d_name[0] == '.')
+		if (de->d_name[0] == '.') {
+			xlog(D_GENERAL, "Skipping dot file %s",
+					de->d_name);
 			continue;
+		}
 
 		count += nsm_load_host(directory, de->d_name, func);
 	}

[PATCH 2/3] libnsm.a: Replace __attribute_noinline__

[Chuck Lever]

"Gabor Z. Papp" <gzp@papp.hu> reports:

trying to compile nfs-utils 1.2.3 on linux kernel 2.4.37.10, glibc 2.2.5:

cc -DHAVE_CONFIG_H -I. -I../../support/include   -D_GNU_SOURCE -Wall
-Wextra -Wstrict-prototypes  -pipe -g -O2 -MT file.o -MD -MP -MF
.deps/file.Tpo -c -o file.o file.c
file.c:638: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'static'
file.c: In function 'nsm_insert_monitored_host':
file.c:747: warning: implicit declaration of function 'nsm_create_monitor_record'
file.c: At top level:
file.c:788: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'static'
file.c: In function 'nsm_read_line':
file.c:842: warning: implicit declaration of function 'nsm_parse_line'
make[3]: *** [file.o] Error 1
make[3]: Leaving directory `/home/gzp/src/nfs-utils-1.2.3/support/nsm'
make[2]: *** [all] Error 2
make[2]: Leaving directory `/home/gzp/src/nfs-utils-1.2.3/support/nsm'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/gzp/src/nfs-utils-1.2.3/support'
make: *** [all-recursive] Error 1

[kernel]
Linux gzpLinux 2.4.37.10-gzpLinux #1 Mon Oct 4 08:57:02 CEST 2010 i686 GNU/Linux

[glibc]
GNU C Library stable release version 2.2.5, by Roland McGrath et al.
Compiled by GNU CC version 3.3.6.
Compiled on a Linux 2.4.36.2-gzpLinux system on 2008-03-22.

[gcc]
gcc (GCC) 4.4.5

[binutils]
GNU ld (Linux/GNU Binutils) 2.21.51.0.1.20101110

=== cut here ===

sys/cdefs.c in glibc 2.2.5 does not define __attribute_noinline__.

Replace the __attribute_noinline__ form with

  __attribute__((__noinline__)).

Even though the compiler didn't complain about __attribute_malloc__,
also replace those in order to maintain consistent style throughout the
source file.

Fix for:

  https://bugzilla.linux-nfs.org/show_bug.cgi?id=194

Reported-by: "Gabor Z. Papp" <gzp@papp.hu>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 support/nsm/file.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/support/nsm/file.c b/support/nsm/file.c
index ba2a9ce..6ea6ab6 100644
--- a/support/nsm/file.c
+++ b/support/nsm/file.c
@@ -126,7 +126,7 @@ exact_error_check(const ssize_t len, const size_t buflen)
  * containing an appropriate pathname, or NULL if an error
  * occurs.  Caller must free the returned result with free(3).
  */
-__attribute_malloc__
+__attribute__((__malloc__))
 static char *
 nsm_make_record_pathname(const char *directory, const char *hostname)
 {
@@ -174,7 +174,7 @@ nsm_make_record_pathname(const char *directory, const char *hostname)
  * containing an appropriate pathname, or NULL if an error
  * occurs.  Caller must free the returned result with free(3).
  */
-__attribute_malloc__
+__attribute__((__malloc__))
 static char *
 nsm_make_pathname(const char *directory)
 {
@@ -204,7 +204,7 @@ nsm_make_pathname(const char *directory)
  * containing an appropriate pathname, or NULL if an error
  * occurs.  Caller must free the returned result with free(3).
  */
-__attribute_malloc__
+__attribute__((__malloc__))
 static char *
 nsm_make_temp_pathname(const char *pathname)
 {
@@ -650,7 +650,7 @@ nsm_priv_to_hex(const char *priv, char *buf, const size_t buflen)
 /*
  * Returns the length in bytes of the created record.
  */
-__attribute_noinline__
+__attribute__((__noinline__))
 static size_t
 nsm_create_monitor_record(char *buf, const size_t buflen,
 		const struct sockaddr *sap, const struct mon *m)
@@ -800,7 +800,7 @@ out:
 	return result;
 }
 
-__attribute_noinline__
+__attribute__((__noinline__))
 static _Bool
 nsm_parse_line(char *line, struct sockaddr_in *sin, struct mon *m)
 {

[PATCH 3/3] sm-notify: Make use of AI_NUMERICSERV conditional

[Chuck Lever]

Gabor Papp reports nfs-utils-1.2.3 doesn't build on his system that
uses glibc-2.2.5:

make[3]: Entering directory
`/home/gzp/src/nfs-utils-1.2.3/utils/statd'
gcc -DHAVE_CONFIG_H -I. -I../../support/include   -D_GNU_SOURCE -Wall
	-Wextra -Wstrict-prototypes  -pipe -g -O2 -MT sm-notify.o -MD
	-MP -MF .deps/sm-notify.Tpo -c -o sm-notify.o sm-notify.c
	sm-notify.c: In function 'smn_bind_address':
sm-notify.c:247: error: 'AI_NUMERICSERV' undeclared (first use in this function)
sm-notify.c:247: error: (Each undeclared identifier is reported only once
sm-notify.c:247: error: for each function it appears in.)
make[3]: *** [sm-notify.o] Error 1

According to the getaddrinfo(3) man page, AI_NUMERICSERV is available
only since glibc 2.3.4.  getaddrinfo(3) seems to convert strings
containing a number to the right port value without the use of
AI_NUMERICSERV, so I think we can survive on older glibc's without it.
It will allow admins to specify service names as well as port numbers
on those versions.

There are uses of AI_NUMERICSERV in gssd and in nfs_svc_create().  The
one in nfs_svc_create() is behind HAVE_LIBTIRPC, and the other is a
issue only for those who want to deploy Kerberos -- likely in both
cases, a more modern glibc will be present.  I'm going to leave those
two.

Fix for:

  https://bugzilla.linux-nfs.org/show_bug.cgi?id=195

Reported-by: "Gabor Z. Papp" <gzp@papp.hu>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 utils/statd/sm-notify.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/utils/statd/sm-notify.c b/utils/statd/sm-notify.c
index b7f4371..1f490b0 100644
--- a/utils/statd/sm-notify.c
+++ b/utils/statd/sm-notify.c
@@ -34,6 +34,11 @@
 #include "nsm.h"
 #include "nfsrpc.h"
 
+/* glibc before 2.3.4 */
+#ifndef AI_NUMERICSERV
+#define AI_NUMERICSERV	0
+#endif
+
 #define NSM_TIMEOUT	2
 #define NSM_MAX_TIMEOUT	120	/* don't make this too big */
 
@@ -248,6 +253,7 @@ smn_bind_address(const char *srcaddr, const char *srcport)
 	if (srcaddr == NULL)
 		hint.ai_flags |= AI_PASSIVE;
 
+	/* Do not allow "node" and "service" parameters both to be NULL */
 	if (srcport == NULL)
 		error = getaddrinfo(srcaddr, "", &hint, &ai);
 	else

Re: [PATCH 1/3] libnsm.a: sm-notify sometimes ignores monitored hosts

[Steve Dickson]

Hey Chuck,

On 12/06/2010 11:09 AM, Chuck Lever wrote:
> Monitored host information is stored in files under /var/lib/nfs.
> When visiting entries in the monitored hosts directory, libnsm.a
> examines the value of dirent.d_type to determine if an entry is a
> regular file.
> 
> According to readdir(3), the d_type field is not supported by all
> file system types.  My root file system happens to be one where d_type
> isn't supported.  Typical installations that use an ext-derived root
> file system are not exposed to this issue, but those who use xfs, for
> instance, are.
> 
> On such file systems, not only are remote peers not notified of
> reboots, but the NSM state number is never incremented.  A statd warm
> restart would not re-monitor any hosts that were monitored before
> the restart.
> 
> When writing support/nsm/file.c, I copied the use of d_type from the
> original statd code, so this has likely been an issue for some time.
> 
> Replace the use of d_type in support/nsm/file.c with a call to
> lstat(2).  It's extra code, but is guaranteed to work on all file
> system types.
> 
> Note there is a usage of d_type in gssd.  I'll let gssd and rpcpipefs
> experts decide whether that's worth changing.
> 
> Fix for:
> 
>   https://bugzilla.linux-nfs.org/show_bug.cgi?id=193
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  support/nsm/file.c |   38 ++++++++++++++++++++++++++++++--------
>  1 files changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/support/nsm/file.c b/support/nsm/file.c
> index f4baeb9..ba2a9ce 100644
> --- a/support/nsm/file.c
> +++ b/support/nsm/file.c
> @@ -568,11 +568,13 @@ nsm_retire_monitored_hosts(void)
>  
>  	while ((de = readdir(dir)) != NULL) {
>  		char *src, *dst;
> +		struct stat stb;
>  
> -		if (de->d_type != (unsigned char)DT_REG)
> -			continue;
> -		if (de->d_name[0] == '.')
> +		if (de->d_name[0] == '.') {
> +			xlog(D_GENERAL, "Skipping dot file %s",
> +					de->d_name);
>  			continue;
> +		}
>  
>  		src = nsm_make_record_pathname(NSM_MONITOR_DIR, de->d_name);
>  		if (src == NULL) {
> @@ -580,6 +582,20 @@ nsm_retire_monitored_hosts(void)
>  			continue;
>  		}
>  
> +		/* NB: not all file systems fill in d_type correctly */
> +		if (lstat(src, &stb) == -1) {
> +			xlog_warn("Bad monitor file %s, skipping: %m",
> +					de->d_name);
> +			free(src);
> +			continue;
> +		}
> +		if (!S_ISREG(stb.st_mode)) {
> +			xlog(D_GENERAL, "Skipping non-regular file %s",
> +					de->d_name);
> +			free(src);
> +			continue;
> +		}
> +
>  		dst = nsm_make_record_pathname(NSM_NOTIFY_DIR, de->d_name);
>  		if (dst == NULL) {
>  			free(src);
> @@ -846,7 +862,7 @@ nsm_read_line(const char *hostname, const time_t timestamp, char *line,
>  }
>  
>  /*
> - * Given a filename, reads data from a file under NSM_MONITOR_DIR
> + * Given a filename, reads data from a file under "directory"
>   * and invokes @func so caller can populate their in-core
>   * database with this data.
>   */
> @@ -863,10 +879,15 @@ nsm_load_host(const char *directory, const char *filename, nsm_populate_t func)
>  	if (path == NULL)
>  		goto out_err;
>  
> -	if (stat(path, &stb) == -1) {
> +	if (lstat(path, &stb) == -1) {
>  		xlog(L_ERROR, "Failed to stat %s: %m", path);
>  		goto out_freepath;
>  	}
> +	if (!S_ISREG(stb.st_mode)) {
> +		xlog(D_GENERAL, "Skipping non-regular file %s",
> +				path);
Question, why do we care non-regular files are being ignored?

I understand logging the lstat() error but logging statements like
"ignoring this" or "not doing that" just make the debug output a 
bit too noisy IMHO... 

I'm thinking it would be better to log the files that 
are actually being used verses files that are being ignored.


> +		goto out_freepath;
> +	}
>  
>  	f = fopen(path, "r");
>  	if (f == NULL) {
> @@ -913,10 +934,11 @@ nsm_load_dir(const char *directory, nsm_populate_t func)
>  	}
>  
>  	while ((de = readdir(dir)) != NULL) {
> -		if (de->d_type != (unsigned char)DT_REG)
> -			continue;
> -		if (de->d_name[0] == '.')
> +		if (de->d_name[0] == '.') {
> +			xlog(D_GENERAL, "Skipping dot file %s",
> +					de->d_name);
The same goes with this... Do we really care "." and ".." are
being ignored? Isn't that expected? ;-)

steved.

>  			continue;
> +		}
>  
>  		count += nsm_load_host(directory, de->d_name, func);
>  	}
> 

Re: [PATCH 1/3] libnsm.a: sm-notify sometimes ignores monitored hosts

[Chuck Lever]

On Dec 13, 2010, at 11:54 AM, Steve Dickson wrote:

> Hey Chuck,
> 
> On 12/06/2010 11:09 AM, Chuck Lever wrote:
>> Monitored host information is stored in files under /var/lib/nfs.
>> When visiting entries in the monitored hosts directory, libnsm.a
>> examines the value of dirent.d_type to determine if an entry is a
>> regular file.
>> 
>> According to readdir(3), the d_type field is not supported by all
>> file system types.  My root file system happens to be one where d_type
>> isn't supported.  Typical installations that use an ext-derived root
>> file system are not exposed to this issue, but those who use xfs, for
>> instance, are.
>> 
>> On such file systems, not only are remote peers not notified of
>> reboots, but the NSM state number is never incremented.  A statd warm
>> restart would not re-monitor any hosts that were monitored before
>> the restart.
>> 
>> When writing support/nsm/file.c, I copied the use of d_type from the
>> original statd code, so this has likely been an issue for some time.
>> 
>> Replace the use of d_type in support/nsm/file.c with a call to
>> lstat(2).  It's extra code, but is guaranteed to work on all file
>> system types.
>> 
>> Note there is a usage of d_type in gssd.  I'll let gssd and rpcpipefs
>> experts decide whether that's worth changing.
>> 
>> Fix for:
>> 
>>  https://bugzilla.linux-nfs.org/show_bug.cgi?id=193
>> 
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> 
>> support/nsm/file.c |   38 ++++++++++++++++++++++++++++++--------
>> 1 files changed, 30 insertions(+), 8 deletions(-)
>> 
>> diff --git a/support/nsm/file.c b/support/nsm/file.c
>> index f4baeb9..ba2a9ce 100644
>> --- a/support/nsm/file.c
>> +++ b/support/nsm/file.c
>> @@ -568,11 +568,13 @@ nsm_retire_monitored_hosts(void)
>> 
>> 	while ((de = readdir(dir)) != NULL) {
>> 		char *src, *dst;
>> +		struct stat stb;
>> 
>> -		if (de->d_type != (unsigned char)DT_REG)
>> -			continue;
>> -		if (de->d_name[0] == '.')
>> +		if (de->d_name[0] == '.') {
>> +			xlog(D_GENERAL, "Skipping dot file %s",
>> +					de->d_name);
>> 			continue;
>> +		}
>> 
>> 		src = nsm_make_record_pathname(NSM_MONITOR_DIR, de->d_name);
>> 		if (src == NULL) {
>> @@ -580,6 +582,20 @@ nsm_retire_monitored_hosts(void)
>> 			continue;
>> 		}
>> 
>> +		/* NB: not all file systems fill in d_type correctly */
>> +		if (lstat(src, &stb) == -1) {
>> +			xlog_warn("Bad monitor file %s, skipping: %m",
>> +					de->d_name);
>> +			free(src);
>> +			continue;
>> +		}
>> +		if (!S_ISREG(stb.st_mode)) {
>> +			xlog(D_GENERAL, "Skipping non-regular file %s",
>> +					de->d_name);
>> +			free(src);
>> +			continue;
>> +		}
>> +
>> 		dst = nsm_make_record_pathname(NSM_NOTIFY_DIR, de->d_name);
>> 		if (dst == NULL) {
>> 			free(src);
>> @@ -846,7 +862,7 @@ nsm_read_line(const char *hostname, const time_t timestamp, char *line,
>> }
>> 
>> /*
>> - * Given a filename, reads data from a file under NSM_MONITOR_DIR
>> + * Given a filename, reads data from a file under "directory"
>>  * and invokes @func so caller can populate their in-core
>>  * database with this data.
>>  */
>> @@ -863,10 +879,15 @@ nsm_load_host(const char *directory, const char *filename, nsm_populate_t func)
>> 	if (path == NULL)
>> 		goto out_err;
>> 
>> -	if (stat(path, &stb) == -1) {
>> +	if (lstat(path, &stb) == -1) {
>> 		xlog(L_ERROR, "Failed to stat %s: %m", path);
>> 		goto out_freepath;
>> 	}
>> +	if (!S_ISREG(stb.st_mode)) {
>> +		xlog(D_GENERAL, "Skipping non-regular file %s",
>> +				path);
> Question, why do we care non-regular files are being ignored?

We probably want to report anything unexpected in the /var/lib/nfs/statd/sm{,.bak} directories.

> I understand logging the lstat() error but logging statements like
> "ignoring this" or "not doing that" just make the debug output a 
> bit too noisy IMHO... 

My expectation is that under normal circumstances this message would never fire.  statd shouldn't put anything in that directory that isn't a regular file.  If there's something else in there, we should report it.  Also, if statd (or something else) is broken and the code thinks the object isn't a regular file, then this message would point to what is wrong.

> I'm thinking it would be better to log the files that 
> are actually being used verses files that are being ignored.

I'd have to check, but I think the hostnames that are "used" are actually reported by the caller.  If callers don't report the hostnames, then we can add something here easily enough.

>> +		goto out_freepath;
>> +	}
>> 
>> 	f = fopen(path, "r");
>> 	if (f == NULL) {
>> @@ -913,10 +934,11 @@ nsm_load_dir(const char *directory, nsm_populate_t func)
>> 	}
>> 
>> 	while ((de = readdir(dir)) != NULL) {
>> -		if (de->d_type != (unsigned char)DT_REG)
>> -			continue;
>> -		if (de->d_name[0] == '.')
>> +		if (de->d_name[0] == '.') {
>> +			xlog(D_GENERAL, "Skipping dot file %s",
>> +					de->d_name);
> The same goes with this... Do we really care "." and ".." are
> being ignored? Isn't that expected? ;-)

It's kind of a heartbeat message, if you will.  But this one is optional and can be removed.

Once we agree on what messages should appear here, I'll redrive the patch.

> 
> steved.
> 
>> 			continue;
>> +		}
>> 
>> 		count += nsm_load_host(directory, de->d_name, func);
>> 	}
>> 

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

Re: [PATCH 1/3] libnsm.a: sm-notify sometimes ignores monitored hosts

[Steve Dickson]

Hey...

On 12/13/2010 12:08 PM, Chuck Lever wrote:

>>>  */
>>> @@ -863,10 +879,15 @@ nsm_load_host(const char *directory, const char *filename, nsm_populate_t func)
>>> 	if (path == NULL)
>>> 		goto out_err;
>>>
>>> -	if (stat(path, &stb) == -1) {
>>> +	if (lstat(path, &stb) == -1) {
>>> 		xlog(L_ERROR, "Failed to stat %s: %m", path);
>>> 		goto out_freepath;
>>> 	}
>>> +	if (!S_ISREG(stb.st_mode)) {
>>> +		xlog(D_GENERAL, "Skipping non-regular file %s",
>>> +				path);
>> Question, why do we care non-regular files are being ignored?
> 
> We probably want to report anything unexpected in 
> the /var/lib/nfs/statd/sm{,.bak} directories.
> 
>> I understand logging the lstat() error but logging statements like
>> "ignoring this" or "not doing that" just make the debug output a 
>> bit too noisy IMHO... 
> 
> My expectation is that under normal circumstances this message 
> would never fire.  
Correct the only way they will be shown is with the "-F -d" flags... 

> statd shouldn't put anything in that directory 
> that isn't a regular file.  If there's something else in there, 
> we should report it.  Also, if statd (or something else) is broken 
> and the code thinks the object isn't a regular file, then this 
> message would point to what is wrong.
To put this in context, here are the message that come up with
the patch applied... I added a symlink to /var/lib/nfs/statd/sm/statd
to generate both messages... 

statd: Version 1.2.3 starting
statd: Flags: No-Daemon Log-STDERR TI-RPC 
sm-notify: Version 1.2.3 starting
sm-notify: Already notifying clients; Exiting!
statd: Skipping dot file ..
statd: Skipping non-regular file /var/lib/nfs/statd/sm/statd
statd: Skipping dot file .

I think we both agree the "Skipping dot file" simply not
needed... The "Skipping non-regular" message... well I have 
to say it really does not have much meaning... either.. IMHO...

But I do agree, it probably will *never* seen... 

steved.

Re: [PATCH 1/3] libnsm.a: sm-notify sometimes ignores monitored hosts

[Steve Dickson]

On 12/13/2010 02:32 PM, Steve Dickson wrote:
> Hey...
> 
> On 12/13/2010 12:08 PM, Chuck Lever wrote:
> 
>>>>  */
>>>> @@ -863,10 +879,15 @@ nsm_load_host(const char *directory, const char *filename, nsm_populate_t func)
>>>> 	if (path == NULL)
>>>> 		goto out_err;
>>>>
>>>> -	if (stat(path, &stb) == -1) {
>>>> +	if (lstat(path, &stb) == -1) {
>>>> 		xlog(L_ERROR, "Failed to stat %s: %m", path);
>>>> 		goto out_freepath;
>>>> 	}
>>>> +	if (!S_ISREG(stb.st_mode)) {
>>>> +		xlog(D_GENERAL, "Skipping non-regular file %s",
>>>> +				path);
>>> Question, why do we care non-regular files are being ignored?
>>
>> We probably want to report anything unexpected in 
>> the /var/lib/nfs/statd/sm{,.bak} directories.
>>
>>> I understand logging the lstat() error but logging statements like
>>> "ignoring this" or "not doing that" just make the debug output a 
>>> bit too noisy IMHO... 
>>
>> My expectation is that under normal circumstances this message 
>> would never fire.  
> Correct the only way they will be shown is with the "-F -d" flags... 
> 
>> statd shouldn't put anything in that directory 
>> that isn't a regular file.  If there's something else in there, 
>> we should report it.  Also, if statd (or something else) is broken 
>> and the code thinks the object isn't a regular file, then this 
>> message would point to what is wrong.
> To put this in context, here are the message that come up with
> the patch applied... I added a symlink to /var/lib/nfs/statd/sm/statd
> to generate both messages... 
> 
> statd: Version 1.2.3 starting
> statd: Flags: No-Daemon Log-STDERR TI-RPC 
> sm-notify: Version 1.2.3 starting
> sm-notify: Already notifying clients; Exiting!
> statd: Skipping dot file ..
> statd: Skipping non-regular file /var/lib/nfs/statd/sm/statd
> statd: Skipping dot file .

This does fix true bug, so I am going to commit the patch 
but without the "Skipping dot file" statements and leaving
the "Skipping non-regular" ones since they will never be
seen...

steved.

Re: [PATCH 1/3] libnsm.a: sm-notify sometimes ignores monitored hosts

[Steve Dickson]

On 12/06/2010 11:09 AM, Chuck Lever wrote:
> Monitored host information is stored in files under /var/lib/nfs.
> When visiting entries in the monitored hosts directory, libnsm.a
> examines the value of dirent.d_type to determine if an entry is a
> regular file.
> 
> According to readdir(3), the d_type field is not supported by all
> file system types.  My root file system happens to be one where d_type
> isn't supported.  Typical installations that use an ext-derived root
> file system are not exposed to this issue, but those who use xfs, for
> instance, are.
> 
> On such file systems, not only are remote peers not notified of
> reboots, but the NSM state number is never incremented.  A statd warm
> restart would not re-monitor any hosts that were monitored before
> the restart.
> 
> When writing support/nsm/file.c, I copied the use of d_type from the
> original statd code, so this has likely been an issue for some time.
> 
> Replace the use of d_type in support/nsm/file.c with a call to
> lstat(2).  It's extra code, but is guaranteed to work on all file
> system types.
> 
> Note there is a usage of d_type in gssd.  I'll let gssd and rpcpipefs
> experts decide whether that's worth changing.
> 
> Fix for:
> 
>   https://bugzilla.linux-nfs.org/show_bug.cgi?id=193
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Committed... But without the "Skipping dot file..." xlogs.

steved

Re: [PATCH 2/3] libnsm.a: Replace __attribute_noinline__

[Steve Dickson]

On 12/06/2010 11:09 AM, Chuck Lever wrote:
> "Gabor Z. Papp" <gzp-2g/1Y3AqmNE@public.gmane.org> reports:
> 
> trying to compile nfs-utils 1.2.3 on linux kernel 2.4.37.10, glibc 2.2.5:
> 
> cc -DHAVE_CONFIG_H -I. -I../../support/include   -D_GNU_SOURCE -Wall
> -Wextra -Wstrict-prototypes  -pipe -g -O2 -MT file.o -MD -MP -MF
> .deps/file.Tpo -c -o file.o file.c
> file.c:638: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'static'
> file.c: In function 'nsm_insert_monitored_host':
> file.c:747: warning: implicit declaration of function 'nsm_create_monitor_record'
> file.c: At top level:
> file.c:788: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'static'
> file.c: In function 'nsm_read_line':
> file.c:842: warning: implicit declaration of function 'nsm_parse_line'
> make[3]: *** [file.o] Error 1
> make[3]: Leaving directory `/home/gzp/src/nfs-utils-1.2.3/support/nsm'
> make[2]: *** [all] Error 2
> make[2]: Leaving directory `/home/gzp/src/nfs-utils-1.2.3/support/nsm'
> make[1]: *** [all-recursive] Error 1
> make[1]: Leaving directory `/home/gzp/src/nfs-utils-1.2.3/support'
> make: *** [all-recursive] Error 1
> 
> [kernel]
> Linux gzpLinux 2.4.37.10-gzpLinux #1 Mon Oct 4 08:57:02 CEST 2010 i686 GNU/Linux
> 
> [glibc]
> GNU C Library stable release version 2.2.5, by Roland McGrath et al.
> Compiled by GNU CC version 3.3.6.
> Compiled on a Linux 2.4.36.2-gzpLinux system on 2008-03-22.
> 
> [gcc]
> gcc (GCC) 4.4.5
> 
> [binutils]
> GNU ld (Linux/GNU Binutils) 2.21.51.0.1.20101110
> 
> === cut here ===
> 
> sys/cdefs.c in glibc 2.2.5 does not define __attribute_noinline__.
> 
> Replace the __attribute_noinline__ form with
> 
>   __attribute__((__noinline__)).
> 
> Even though the compiler didn't complain about __attribute_malloc__,
> also replace those in order to maintain consistent style throughout the
> source file.
> 
> Fix for:
> 
>   https://bugzilla.linux-nfs.org/show_bug.cgi?id=194
> 
> Reported-by: "Gabor Z. Papp" <gzp-2g/1Y3AqmNE@public.gmane.org>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Committed.. 

steved.

Re: [PATCH 3/3] sm-notify: Make use of AI_NUMERICSERV conditional

[Steve Dickson]

On 12/06/2010 11:10 AM, Chuck Lever wrote:
> Gabor Papp reports nfs-utils-1.2.3 doesn't build on his system that
> uses glibc-2.2.5:
> 
> make[3]: Entering directory
> `/home/gzp/src/nfs-utils-1.2.3/utils/statd'
> gcc -DHAVE_CONFIG_H -I. -I../../support/include   -D_GNU_SOURCE -Wall
> 	-Wextra -Wstrict-prototypes  -pipe -g -O2 -MT sm-notify.o -MD
> 	-MP -MF .deps/sm-notify.Tpo -c -o sm-notify.o sm-notify.c
> 	sm-notify.c: In function 'smn_bind_address':
> sm-notify.c:247: error: 'AI_NUMERICSERV' undeclared (first use in this function)
> sm-notify.c:247: error: (Each undeclared identifier is reported only once
> sm-notify.c:247: error: for each function it appears in.)
> make[3]: *** [sm-notify.o] Error 1
> 
> According to the getaddrinfo(3) man page, AI_NUMERICSERV is available
> only since glibc 2.3.4.  getaddrinfo(3) seems to convert strings
> containing a number to the right port value without the use of
> AI_NUMERICSERV, so I think we can survive on older glibc's without it.
> It will allow admins to specify service names as well as port numbers
> on those versions.
> 
> There are uses of AI_NUMERICSERV in gssd and in nfs_svc_create().  The
> one in nfs_svc_create() is behind HAVE_LIBTIRPC, and the other is a
> issue only for those who want to deploy Kerberos -- likely in both
> cases, a more modern glibc will be present.  I'm going to leave those
> two.
> 
> Fix for:
> 
>   https://bugzilla.linux-nfs.org/show_bug.cgi?id=195
> 
> Reported-by: "Gabor Z. Papp" <gzp-2g/1Y3AqmNE@public.gmane.org>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Committed... 

steved.