[PATCH 0/2] Added error logging and verbosity flag (ver #2)

[PATCH 0/2] Added error logging and verbosity flag (ver #2)

[Steve Dickson]

Here the second version of this patch series. In this
version, as suggested, nfsidmap now uses getopt()
to parse the command line arguments. While I was
there I so convert the timeout value to be specified
by '-t' flag instead of being the last value on the
command line.

I also prove this new debugging is helpful and needed,
because it help me find a bug in the libnfsidmap lib.

Steve Dickson (2):
  nfsidmap: Added Error Logging
  nfsidmap: Added -v and -t flags

 utils/nfsidmap/Makefile.am  |    2 +-
 utils/nfsidmap/nfsidmap.c   |   77 +++++++++++++++++++++++++++++++++---------
 utils/nfsidmap/nfsidmap.man |   26 ++++++++++----
 3 files changed, 79 insertions(+), 26 deletions(-)

-- 
1.7.7

[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.7

[PATCH 2/2] nfsidmap: Added -v and -t flags

[Steve Dickson]

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

Also converted the timeout argument to use a -t flag.

Signed-off-by: Steve Dickson <steved@redhat.com>
---
 utils/nfsidmap/nfsidmap.c   |   45 +++++++++++++++++++++++++++++-------------
 utils/nfsidmap/nfsidmap.man |   26 +++++++++++++++++-------
 2 files changed, 49 insertions(+), 22 deletions(-)

diff --git a/utils/nfsidmap/nfsidmap.c b/utils/nfsidmap/nfsidmap.c
index 134d9bc..6f85c5f 100644
--- a/utils/nfsidmap/nfsidmap.c
+++ b/utils/nfsidmap/nfsidmap.c
@@ -9,17 +9,17 @@
 #include <keyutils.h>
 #include <nfsidmap.h>
 
-#include <syslog.h>
+#include <unistd.h>
 #include "xlog.h"
 
-/* gcc nfsidmap.c -o nfsidmap -l nfsidmap -l keyutils */
+int verbose = 0;
+char *usage="Usage: %s [-v] [-t timeout] key desc";
 
 #define MAX_ID_LEN   11
 #define IDMAP_NAMESZ 128
 #define USER  1
 #define GROUP 0
 
-
 /*
  * Find either a user or group id based on the name@domain string
  */
@@ -93,7 +93,7 @@ int main(int argc, char **argv)
 	char *arg;
 	char *value;
 	char *type;
-	int rc = 1;
+	int rc = 1, opt;
 	int timeout = 600;
 	key_serial_t key;
 	char *progname;
@@ -108,24 +108,41 @@ int main(int argc, char **argv)
 	xlog_syslog(1);
 	xlog_stderr(0);
 
-	if (argc < 3) {
+	while ((opt = getopt(argc, argv, "t:v")) != -1) {
+		switch (opt) {
+		case 'v':
+			verbose++;
+			break;
+		case 't':
+			timeout = atoi(optarg);
+			break;
+		default:
+			xlog_warn(usage, progname);
+			break;
+		}
+	}
+
+	if ((argc - optind) != 2) {
 		xlog_err("Bad arg count. Check /etc/request-key.conf");
+		xlog_warn(usage, progname);
 		return 1;
 	}
 
-	arg = malloc(sizeof(char) * strlen(argv[2]) + 1);
-	strcpy(arg, argv[2]);
+	if (verbose)
+		nfs4_set_debug(verbose, NULL);
+
+	key = strtol(argv[optind++], NULL, 10);
+
+	arg = malloc(sizeof(char) * strlen(argv[optind]) + 1);
+	strcpy(arg, argv[optind]);
 	type = strtok(arg, ":");
 	value = strtok(NULL, ":");
 
-	if (argc == 4) {
-		timeout = atoi(argv[3]);
-		if (timeout < 0)
-			timeout = 0;
+	if (verbose) {
+		xlog_warn("key: %ld type: %s value: %s timeout %ld",
+			key, type, value, timeout);
 	}
 
-	key = strtol(argv[1], NULL, 10);
-
 	if (strcmp(type, "uid") == 0)
 		rc = id_lookup(value, key, USER);
 	else if (strcmp(type, "gid") == 0)
@@ -135,7 +152,7 @@ int main(int argc, char **argv)
 	else if (strcmp(type, "group") == 0)
 		rc = name_lookup(value, key, GROUP);
 
-	/* Set timeout to 5 (600 seconds) minutes */
+	/* Set timeout to 10 (600 seconds) minutes */
 	if (rc == 0)
 		keyctl_set_timeout(key, timeout);
 
diff --git a/utils/nfsidmap/nfsidmap.man b/utils/nfsidmap/nfsidmap.man
index 2381908..c67aab6 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] [-t timeout] key desc"
 .SH DESCRIPTION
 The file
 .I /usr/sbin/nfsidmap
@@ -14,9 +16,15 @@ 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 -t timeout
+Set the expiration timer, in seconds, on the key.
+The default is 600 seconds (10 mins).
+.TP
+.B -v
+Increases the verbosity of the output to syslog 
+(can be specified multiple times).
 .SH CONFIGURING
 The file
 .I /etc/request-key.conf
@@ -25,11 +33,13 @@ will need to be modified so
 can properly direct the upcall. The following line should be added before a call
 to keyctl negate:
 .PP
-create	id_resolver	*	*	/usr/sbin/nfsidmap %k %d 600
+create	id_resolver	*	*	/usr/sbin/nfsidmap -t 600 %k %d 
 .PP
 This will direct all id_resolver requests to the program
-.I /usr/sbin/nfsidmap
-The last parameter, 600, defines how many seconds into the future the key will
+.I /usr/sbin/nfsidmap.
+The 
+.B -t 600 
+defines how many seconds into the future the key will
 expire.  This is an optional parameter for
 .I /usr/sbin/nfsidmap
 and will default to 600 seconds when not specified.
@@ -48,9 +58,9 @@ You can choose to handle any of these individually, rather than using the
 generic upcall program.  If you would like to use your own program for a uid
 lookup then you would edit your request-key.conf so it looks similar to this:
 .PP
-create	id_resolver	uid:*	*	/some/other/program %k %d 600
+create	id_resolver	uid:*	*	/some/other/program %k %d
 .br
-create	id_resolver	*		*	/usr/sbin/nfsidmap %k %d 600
+create	id_resolver	*		*	/usr/sbin/nfsidmap %k %d
 .PP
 Notice that the new line was added above the line for the generic program.
 request-key will find the first matching line and run the corresponding program.
-- 
1.7.7

Re: [PATCH 2/2] nfsidmap: Added -v and -t flags

[Jim Rees]

Steve Dickson wrote:

  +	arg = malloc(sizeof(char) * strlen(argv[optind]) + 1);
  +	strcpy(arg, argv[optind]);

strdup() would have been simpler.  But don't re-do it just for this.

Re: [PATCH 2/2] nfsidmap: Added -v and -t flags

[Steve Dickson]

On 11/12/2011 02:15 PM, Jim Rees wrote:
> Steve Dickson wrote:
> 
>   +	arg = malloc(sizeof(char) * strlen(argv[optind]) + 1);
>   +	strcpy(arg, argv[optind]);
> 
> strdup() would have been simpler.  But don't re-do it just for this.
We should also be checking for failure as well... Here's how I took care of it:

-       arg = malloc(sizeof(char) * strlen(argv[2]) + 1);
-       strcpy(arg, argv[2]);
+       if (verbose)
+               nfs4_set_debug(verbose, NULL);
+
+       key = strtol(argv[optind++], NULL, 10);
+
+       arg = strdup(argv[optind]);
+       if (arg == NULL) {
+               xlog_err("strdup failed: %m");
+               return 1;
+       }

steved.

Re: [PATCH 0/2] Added error logging and verbosity flag (ver #2)

[Steve Dickson]

On 11/12/2011 10:55 AM, Steve Dickson wrote:
> Here the second version of this patch series. In this
> version, as suggested, nfsidmap now uses getopt()
> to parse the command line arguments. While I was
> there I so convert the timeout value to be specified
> by '-t' flag instead of being the last value on the
> command line.
> 
> I also prove this new debugging is helpful and needed,
> because it help me find a bug in the libnfsidmap lib.
> 
> Steve Dickson (2):
>   nfsidmap: Added Error Logging
>   nfsidmap: Added -v and -t flags
> 
>  utils/nfsidmap/Makefile.am  |    2 +-
>  utils/nfsidmap/nfsidmap.c   |   77 +++++++++++++++++++++++++++++++++---------
>  utils/nfsidmap/nfsidmap.man |   26 ++++++++++----
>  3 files changed, 79 insertions(+), 26 deletions(-)
> 
Both patches committed....

steved.

[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