[PATCH 0/2] nfsidmap: Added error logging and verbosity flag

[PATCH 0/2] nfsidmap: Added error logging and verbosity flag

[Steve Dickson]

To aid in debugging problems and verifying general
functionality, this patch set adds error logging 
and a -v verbosity flag.

Steve Dickson (2):
  nfsidmap: Added Error Logging
  nfsidmap: Added -v flag

 utils/nfsidmap/Makefile.am  |    2 +-
 utils/nfsidmap/nfsidmap.c   |   46 +++++++++++++++++++++++++++++++++++++++---
 utils/nfsidmap/nfsidmap.man |   15 +++++++++++--
 3 files changed, 55 insertions(+), 8 deletions(-)

-- 
1.7.6.4

[PATCH 1/2] nfsidmap: Added Error Logging

[Steve Dickson]

Since this binary is being called by the kernel,
errors need to be logged to the syslog for
help in debugging problems.

Signed-off-by: Steve Dickson <steved@redhat.com>
---
 utils/nfsidmap/Makefile.am |    2 +-
 utils/nfsidmap/nfsidmap.c  |   34 ++++++++++++++++++++++++++++++----
 2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/utils/nfsidmap/Makefile.am b/utils/nfsidmap/Makefile.am
index f837b91..037aa79 100644
--- a/utils/nfsidmap/Makefile.am
+++ b/utils/nfsidmap/Makefile.am
@@ -4,6 +4,6 @@ man8_MANS = nfsidmap.man
 
 sbin_PROGRAMS	= nfsidmap
 nfsidmap_SOURCES = nfsidmap.c
-nfsidmap_LDADD = -lnfsidmap -lkeyutils
+nfsidmap_LDADD = -lnfsidmap -lkeyutils ../../support/nfs/libnfs.a
 
 MAINTAINERCLEANFILES = Makefile.in
diff --git a/utils/nfsidmap/nfsidmap.c b/utils/nfsidmap/nfsidmap.c
index 2d87381..134d9bc 100644
--- a/utils/nfsidmap/nfsidmap.c
+++ b/utils/nfsidmap/nfsidmap.c
@@ -10,6 +10,7 @@
 #include <nfsidmap.h>
 
 #include <syslog.h>
+#include "xlog.h"
 
 /* gcc nfsidmap.c -o nfsidmap -l nfsidmap -l keyutils */
 
@@ -36,9 +37,15 @@ int id_lookup(char *name_at_domain, key_serial_t key, int type)
 		rc = nfs4_group_owner_to_gid(name_at_domain, &gid);
 		sprintf(id, "%u", gid);
 	}
+	if (rc < 0)
+		xlog_err("id_lookup: %s: failed: %m",
+			(type == USER ? "nfs4_owner_to_uid" : "nfs4_group_owner_to_gid"));
 
-	if (rc == 0)
+	if (rc == 0) {
 		rc = keyctl_instantiate(key, id, strlen(id) + 1, 0);
+		if (rc < 0)
+			xlog_err("id_lookup: keyctl_instantiate failed: %m");
+	}
 
 	return rc;
 }
@@ -57,6 +64,7 @@ int name_lookup(char *id, key_serial_t key, int type)
 	rc = nfs4_get_default_domain(NULL, domain, NFS4_MAX_DOMAIN_LEN);
 	if (rc != 0) {
 		rc = -1;
+		xlog_err("name_lookup: nfs4_get_default_domain failed: %m");
 		goto out;
 	}
 
@@ -67,10 +75,15 @@ int name_lookup(char *id, key_serial_t key, int type)
 		gid = atoi(id);
 		rc = nfs4_gid_to_name(gid, domain, name, IDMAP_NAMESZ);
 	}
+	if (rc < 0)
+		xlog_err("name_lookup: %s: failed: %m",
+			(type == USER ? "nfs4_uid_to_name" : "nfs4_gid_to_name"));
 
-	if (rc == 0)
+	if (rc == 0) {
 		rc = keyctl_instantiate(key, &name, strlen(name), 0);
-
+		if (rc < 0)
+			xlog_err("name_lookup: keyctl_instantiate failed: %m");
+	}
 out:
 	return rc;
 }
@@ -83,9 +96,22 @@ int main(int argc, char **argv)
 	int rc = 1;
 	int timeout = 600;
 	key_serial_t key;
+	char *progname;
+
+	/* Set the basename */
+	if ((progname = strrchr(argv[0], '/')) != NULL)
+		progname++;
+	else
+		progname = argv[0];
 
-	if (argc < 3)
+	xlog_open(progname);
+	xlog_syslog(1);
+	xlog_stderr(0);
+
+	if (argc < 3) {
+		xlog_err("Bad arg count. Check /etc/request-key.conf");
 		return 1;
+	}
 
 	arg = malloc(sizeof(char) * strlen(argv[2]) + 1);
 	strcpy(arg, argv[2]);
-- 
1.7.6.4

[PATCH 2/2] nfsidmap: Added -v flag

[Steve Dickson]

To aid in debugging, the -v flag can now be specified
on the command to enable verbose logging in both
the nfsidmap command and libnfsidmap library routines.

Signed-off-by: Steve Dickson <steved@redhat.com>
---
 utils/nfsidmap/nfsidmap.c   |   12 ++++++++++++
 utils/nfsidmap/nfsidmap.man |   15 ++++++++++++---
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/utils/nfsidmap/nfsidmap.c b/utils/nfsidmap/nfsidmap.c
index 134d9bc..d74189a 100644
--- a/utils/nfsidmap/nfsidmap.c
+++ b/utils/nfsidmap/nfsidmap.c
@@ -12,6 +12,7 @@
 #include <syslog.h>
 #include "xlog.h"
 
+int verbose = 0;
 /* gcc nfsidmap.c -o nfsidmap -l nfsidmap -l keyutils */
 
 #define MAX_ID_LEN   11
@@ -108,6 +109,12 @@ int main(int argc, char **argv)
 	xlog_syslog(1);
 	xlog_stderr(0);
 
+	if (argc > 1 && strcmp(argv[1], "-v") == 0) {
+		verbose = 1;
+		nfs4_set_debug(1, NULL);
+		argc--, argv++;
+	}
+
 	if (argc < 3) {
 		xlog_err("Bad arg count. Check /etc/request-key.conf");
 		return 1;
@@ -126,6 +133,11 @@ int main(int argc, char **argv)
 
 	key = strtol(argv[1], NULL, 10);
 
+	if (verbose) {
+		xlog_warn("key: %ld type: %s value: %s timeout %ld",
+			key, type, value, timeout);
+	}
+
 	if (strcmp(type, "uid") == 0)
 		rc = id_lookup(value, key, USER);
 	else if (strcmp(type, "gid") == 0)
diff --git a/utils/nfsidmap/nfsidmap.man b/utils/nfsidmap/nfsidmap.man
index 2381908..aa4d94b 100644
--- a/utils/nfsidmap/nfsidmap.man
+++ b/utils/nfsidmap/nfsidmap.man
@@ -5,6 +5,8 @@
 .TH nfsidmap 5 "1 October 2010"
 .SH NAME
 nfsidmap \- The NFS idmapper upcall program
+.SH SYNOPSIS
+.B "nfsidmap [-v] key user [timeout]"
 .SH DESCRIPTION
 The file
 .I /usr/sbin/nfsidmap
@@ -14,9 +16,16 @@ the upcall and cache the result.
 .I /usr/sbin/nfsidmap
 should only be called by request-key, and will perform the translation and
 initialize a key with the resulting information.
-.PP
-NFS_USE_NEW_IDMAPPER must be selected when configuring the kernel to use this
-feature.
+.SH OPTIONS
+.TP
+.B -v
+Enables verbose logging in both the
+.B nfsidmap
+binary and in the library routines
+that are used.
+.B Note,
+the -v flag has to be the first argument on the
+command to enable the logging.
 .SH CONFIGURING
 The file
 .I /etc/request-key.conf
-- 
1.7.6.4

Re: [PATCH 2/2] nfsidmap: Added -v flag

[Jim Rees]

Steve Dickson wrote:

  To aid in debugging, the -v flag can now be specified
  on the command to enable verbose logging in both
  the nfsidmap command and libnfsidmap library routines.
  
  Signed-off-by: Steve Dickson <steved@redhat.com>
  ---
   utils/nfsidmap/nfsidmap.c   |   12 ++++++++++++
   utils/nfsidmap/nfsidmap.man |   15 ++++++++++++---
   2 files changed, 24 insertions(+), 3 deletions(-)
  
  diff --git a/utils/nfsidmap/nfsidmap.c b/utils/nfsidmap/nfsidmap.c
  index 134d9bc..d74189a 100644
  --- a/utils/nfsidmap/nfsidmap.c
  +++ b/utils/nfsidmap/nfsidmap.c
  @@ -12,6 +12,7 @@
   #include <syslog.h>
   #include "xlog.h"
   
  +int verbose = 0;
   /* gcc nfsidmap.c -o nfsidmap -l nfsidmap -l keyutils */
   
   #define MAX_ID_LEN   11
  @@ -108,6 +109,12 @@ int main(int argc, char **argv)
   	xlog_syslog(1);
   	xlog_stderr(0);
   
  +	if (argc > 1 && strcmp(argv[1], "-v") == 0) {
  +		verbose = 1;
  +		nfs4_set_debug(1, NULL);
  +		argc--, argv++;
  +	}
  +

Ugh.  Is there some reason not to use getopt() like all the other utils do?

Re: [PATCH 2/2] nfsidmap: Added -v flag

[Bryan Schumaker]

On 11/10/2011 03:26 PM, Steve Dickson wrote:
> To aid in debugging, the -v flag can now be specified
> on the command to enable verbose logging in both
> the nfsidmap command and libnfsidmap library routines.
> 
> Signed-off-by: Steve Dickson <steved@redhat.com>
> ---
>  utils/nfsidmap/nfsidmap.c   |   12 ++++++++++++
>  utils/nfsidmap/nfsidmap.man |   15 ++++++++++++---
>  2 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/utils/nfsidmap/nfsidmap.c b/utils/nfsidmap/nfsidmap.c
> index 134d9bc..d74189a 100644
> --- a/utils/nfsidmap/nfsidmap.c
> +++ b/utils/nfsidmap/nfsidmap.c
> @@ -12,6 +12,7 @@
>  #include <syslog.h>
>  #include "xlog.h"

Is the syslog.h still needed here?  I don't think I see anything that uses it.  If I knew about xlog, I probably would have used that while writing nfsidmap.

- Bryan

>  
> +int verbose = 0;
>  /* gcc nfsidmap.c -o nfsidmap -l nfsidmap -l keyutils */
>  
>  #define MAX_ID_LEN   11
> @@ -108,6 +109,12 @@ int main(int argc, char **argv)
>  	xlog_syslog(1);
>  	xlog_stderr(0);
>  
> +	if (argc > 1 && strcmp(argv[1], "-v") == 0) {
> +		verbose = 1;
> +		nfs4_set_debug(1, NULL);
> +		argc--, argv++;
> +	}
> +
>  	if (argc < 3) {
>  		xlog_err("Bad arg count. Check /etc/request-key.conf");
>  		return 1;
> @@ -126,6 +133,11 @@ int main(int argc, char **argv)
>  
>  	key = strtol(argv[1], NULL, 10);
>  
> +	if (verbose) {
> +		xlog_warn("key: %ld type: %s value: %s timeout %ld",
> +			key, type, value, timeout);
> +	}
> +
>  	if (strcmp(type, "uid") == 0)
>  		rc = id_lookup(value, key, USER);
>  	else if (strcmp(type, "gid") == 0)
> diff --git a/utils/nfsidmap/nfsidmap.man b/utils/nfsidmap/nfsidmap.man
> index 2381908..aa4d94b 100644
> --- a/utils/nfsidmap/nfsidmap.man
> +++ b/utils/nfsidmap/nfsidmap.man
> @@ -5,6 +5,8 @@
>  .TH nfsidmap 5 "1 October 2010"
>  .SH NAME
>  nfsidmap \- The NFS idmapper upcall program
> +.SH SYNOPSIS
> +.B "nfsidmap [-v] key user [timeout]"
>  .SH DESCRIPTION
>  The file
>  .I /usr/sbin/nfsidmap
> @@ -14,9 +16,16 @@ the upcall and cache the result.
>  .I /usr/sbin/nfsidmap
>  should only be called by request-key, and will perform the translation and
>  initialize a key with the resulting information.
> -.PP
> -NFS_USE_NEW_IDMAPPER must be selected when configuring the kernel to use this
> -feature.
> +.SH OPTIONS
> +.TP
> +.B -v
> +Enables verbose logging in both the
> +.B nfsidmap
> +binary and in the library routines
> +that are used.
> +.B Note,
> +the -v flag has to be the first argument on the
> +command to enable the logging.
>  .SH CONFIGURING
>  The file
>  .I /etc/request-key.conf

Re: [PATCH 2/2] nfsidmap: Added -v flag

[Steve Dickson]

On 11/10/2011 04:11 PM, Jim Rees wrote:
> Steve Dickson wrote:
> 
>   To aid in debugging, the -v flag can now be specified
>   on the command to enable verbose logging in both
>   the nfsidmap command and libnfsidmap library routines.
>   
>   Signed-off-by: Steve Dickson <steved@redhat.com>
>   ---
>    utils/nfsidmap/nfsidmap.c   |   12 ++++++++++++
>    utils/nfsidmap/nfsidmap.man |   15 ++++++++++++---
>    2 files changed, 24 insertions(+), 3 deletions(-)
>   
>   diff --git a/utils/nfsidmap/nfsidmap.c b/utils/nfsidmap/nfsidmap.c
>   index 134d9bc..d74189a 100644
>   --- a/utils/nfsidmap/nfsidmap.c
>   +++ b/utils/nfsidmap/nfsidmap.c
>   @@ -12,6 +12,7 @@
>    #include <syslog.h>
>    #include "xlog.h"
>    
>   +int verbose = 0;
>    /* gcc nfsidmap.c -o nfsidmap -l nfsidmap -l keyutils */
>    
>    #define MAX_ID_LEN   11
>   @@ -108,6 +109,12 @@ int main(int argc, char **argv)
>    	xlog_syslog(1);
>    	xlog_stderr(0);
>    
>   +	if (argc > 1 && strcmp(argv[1], "-v") == 0) {
>   +		verbose = 1;
>   +		nfs4_set_debug(1, NULL);
>   +		argc--, argv++;
>   +	}
>   +
> 
> Ugh.  Is there some reason not to use getopt() like all the other utils do?
I was waiting for this comment ;-) 

I decided to go with how the command was originally written
because this command is only call from the kernel so a user
should execute it (except for debugging).  

The arguments are vary static on where they need to be
no command line. So its either going to work or not,
which means there is no real need for a usage error (expect
for the one I added). 

Finally, is there real need for a while loop and switch statement 
for on simple case? I thought not... 

Those are my thoughts.. 

steved.

Re: [PATCH 2/2] nfsidmap: Added -v flag

[Steve Dickson]

On 11/10/2011 04:19 PM, Bryan Schumaker wrote:
> On 11/10/2011 03:26 PM, Steve Dickson wrote:
>> To aid in debugging, the -v flag can now be specified
>> on the command to enable verbose logging in both
>> the nfsidmap command and libnfsidmap library routines.
>>
>> Signed-off-by: Steve Dickson <steved@redhat.com>
>> ---
>>  utils/nfsidmap/nfsidmap.c   |   12 ++++++++++++
>>  utils/nfsidmap/nfsidmap.man |   15 ++++++++++++---
>>  2 files changed, 24 insertions(+), 3 deletions(-)
>>
>> diff --git a/utils/nfsidmap/nfsidmap.c b/utils/nfsidmap/nfsidmap.c
>> index 134d9bc..d74189a 100644
>> --- a/utils/nfsidmap/nfsidmap.c
>> +++ b/utils/nfsidmap/nfsidmap.c
>> @@ -12,6 +12,7 @@
>>  #include <syslog.h>
>>  #include "xlog.h"
> 
> Is the syslog.h still needed here?  I don't think I see anything that uses it.  
Probably not...

> If I knew about xlog, I probably would have used that while writing nfsidmap.
No biggie... :-) 

steved.

> 
> - Bryan
> 
>>  
>> +int verbose = 0;
>>  /* gcc nfsidmap.c -o nfsidmap -l nfsidmap -l keyutils */
>>  
>>  #define MAX_ID_LEN   11
>> @@ -108,6 +109,12 @@ int main(int argc, char **argv)
>>  	xlog_syslog(1);
>>  	xlog_stderr(0);
>>  
>> +	if (argc > 1 && strcmp(argv[1], "-v") == 0) {
>> +		verbose = 1;
>> +		nfs4_set_debug(1, NULL);
>> +		argc--, argv++;
>> +	}
>> +
>>  	if (argc < 3) {
>>  		xlog_err("Bad arg count. Check /etc/request-key.conf");
>>  		return 1;
>> @@ -126,6 +133,11 @@ int main(int argc, char **argv)
>>  
>>  	key = strtol(argv[1], NULL, 10);
>>  
>> +	if (verbose) {
>> +		xlog_warn("key: %ld type: %s value: %s timeout %ld",
>> +			key, type, value, timeout);
>> +	}
>> +
>>  	if (strcmp(type, "uid") == 0)
>>  		rc = id_lookup(value, key, USER);
>>  	else if (strcmp(type, "gid") == 0)
>> diff --git a/utils/nfsidmap/nfsidmap.man b/utils/nfsidmap/nfsidmap.man
>> index 2381908..aa4d94b 100644
>> --- a/utils/nfsidmap/nfsidmap.man
>> +++ b/utils/nfsidmap/nfsidmap.man
>> @@ -5,6 +5,8 @@
>>  .TH nfsidmap 5 "1 October 2010"
>>  .SH NAME
>>  nfsidmap \- The NFS idmapper upcall program
>> +.SH SYNOPSIS
>> +.B "nfsidmap [-v] key user [timeout]"
>>  .SH DESCRIPTION
>>  The file
>>  .I /usr/sbin/nfsidmap
>> @@ -14,9 +16,16 @@ the upcall and cache the result.
>>  .I /usr/sbin/nfsidmap
>>  should only be called by request-key, and will perform the translation and
>>  initialize a key with the resulting information.
>> -.PP
>> -NFS_USE_NEW_IDMAPPER must be selected when configuring the kernel to use this
>> -feature.
>> +.SH OPTIONS
>> +.TP
>> +.B -v
>> +Enables verbose logging in both the
>> +.B nfsidmap
>> +binary and in the library routines
>> +that are used.
>> +.B Note,
>> +the -v flag has to be the first argument on the
>> +command to enable the logging.
>>  .SH CONFIGURING
>>  The file
>>  .I /etc/request-key.conf
> 

Re: [PATCH 2/2] nfsidmap: Added -v flag

[Jim Rees]

Steve Dickson wrote:

  
  
  On 11/10/2011 04:11 PM, Jim Rees wrote:
  > Steve Dickson wrote:
  > 
  >   To aid in debugging, the -v flag can now be specified
  >   on the command to enable verbose logging in both
  >   the nfsidmap command and libnfsidmap library routines.
  >   
  >   Signed-off-by: Steve Dickson <steved@redhat.com>
  >   ---
  >    utils/nfsidmap/nfsidmap.c   |   12 ++++++++++++
  >    utils/nfsidmap/nfsidmap.man |   15 ++++++++++++---
  >    2 files changed, 24 insertions(+), 3 deletions(-)
  >   
  >   diff --git a/utils/nfsidmap/nfsidmap.c b/utils/nfsidmap/nfsidmap.c
  >   index 134d9bc..d74189a 100644
  >   --- a/utils/nfsidmap/nfsidmap.c
  >   +++ b/utils/nfsidmap/nfsidmap.c
  >   @@ -12,6 +12,7 @@
  >    #include <syslog.h>
  >    #include "xlog.h"
  >    
  >   +int verbose = 0;
  >    /* gcc nfsidmap.c -o nfsidmap -l nfsidmap -l keyutils */
  >    
  >    #define MAX_ID_LEN   11
  >   @@ -108,6 +109,12 @@ int main(int argc, char **argv)
  >    	xlog_syslog(1);
  >    	xlog_stderr(0);
  >    
  >   +	if (argc > 1 && strcmp(argv[1], "-v") == 0) {
  >   +		verbose = 1;
  >   +		nfs4_set_debug(1, NULL);
  >   +		argc--, argv++;
  >   +	}
  >   +
  > 
  > Ugh.  Is there some reason not to use getopt() like all the other utils do?
  I was waiting for this comment ;-)

I'm always happy to play your straight man.

  I decided to go with how the command was originally written
  because this command is only call from the kernel so a user
  should execute it (except for debugging).  
  
  The arguments are vary static on where they need to be
  no command line. So its either going to work or not,
  which means there is no real need for a usage error (expect
  for the one I added). 
  
  Finally, is there real need for a while loop and switch statement 
  for on simple case? I thought not...

It's more work for the next guy who comes along and wants to add another
option, especially if the new option takes an argument.

Re: [PATCH 2/2] nfsidmap: Added -v flag

[Steve Dickson]

On 11/10/2011 07:23 PM, Jim Rees wrote:
> Steve Dickson wrote:
> 
>   
>   
>   On 11/10/2011 04:11 PM, Jim Rees wrote:
>   > Steve Dickson wrote:
>   > 
>   >   To aid in debugging, the -v flag can now be specified
>   >   on the command to enable verbose logging in both
>   >   the nfsidmap command and libnfsidmap library routines.
>   >   
>   >   Signed-off-by: Steve Dickson <steved@redhat.com>
>   >   ---
>   >    utils/nfsidmap/nfsidmap.c   |   12 ++++++++++++
>   >    utils/nfsidmap/nfsidmap.man |   15 ++++++++++++---
>   >    2 files changed, 24 insertions(+), 3 deletions(-)
>   >   
>   >   diff --git a/utils/nfsidmap/nfsidmap.c b/utils/nfsidmap/nfsidmap.c
>   >   index 134d9bc..d74189a 100644
>   >   --- a/utils/nfsidmap/nfsidmap.c
>   >   +++ b/utils/nfsidmap/nfsidmap.c
>   >   @@ -12,6 +12,7 @@
>   >    #include <syslog.h>
>   >    #include "xlog.h"
>   >    
>   >   +int verbose = 0;
>   >    /* gcc nfsidmap.c -o nfsidmap -l nfsidmap -l keyutils */
>   >    
>   >    #define MAX_ID_LEN   11
>   >   @@ -108,6 +109,12 @@ int main(int argc, char **argv)
>   >    	xlog_syslog(1);
>   >    	xlog_stderr(0);
>   >    
>   >   +	if (argc > 1 && strcmp(argv[1], "-v") == 0) {
>   >   +		verbose = 1;
>   >   +		nfs4_set_debug(1, NULL);
>   >   +		argc--, argv++;
>   >   +	}
>   >   +
>   > 
>   > Ugh.  Is there some reason not to use getopt() like all the other utils do?
>   I was waiting for this comment ;-)
> 
> I'm always happy to play your straight man.
> 
>   I decided to go with how the command was originally written
>   because this command is only call from the kernel so a user
>   should execute it (except for debugging).  
>   
>   The arguments are vary static on where they need to be
>   no command line. So its either going to work or not,
>   which means there is no real need for a usage error (expect
>   for the one I added). 
>   
>   Finally, is there real need for a while loop and switch statement 
>   for on simple case? I thought not...
> 
> It's more work for the next guy who comes along and wants to add another
> option, especially if the new option takes an argument.
I still think its overkill but I'm always a fan 
of making easier for the next guys down the line...
I'll re-spin it... 

steved.