On Jun 4, 2009, at 4:25 PM, Jeff Layton wrote:
On Thu, 4 Jun 2009 15:38:32 -0400 Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:On Jun 3, 2009, at 3:52 PM, Jeff Layton wrote:...and add a --debug option to make nfsd log messages to stderr. rpc.nfsd is usually run out of init scripts in which case logging to syslog makes more sense. Make that the default and add a switch thatmakes log messages go to stderr. Eventually however nfsd has to closestderr, at which point the program switches to logging to syslog unconditionally. For now, stderr gets closed rather early, so --debug isn't terribly helpful. Later patches will make rpc.nfsd delay closing of stderr longer which should make it more useful. Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx> --- support/nfs/nfssvc.c | 46 +++++++++++++++++------------------- utils/nfsd/nfsd.c | 63 ++++++++++++++++++++++++++++++ +------------------- 2 files changed, 61 insertions(+), 48 deletions(-) diff --git a/support/nfs/nfssvc.c b/support/nfs/nfssvc.c index 33c15a7..3e6bd31 100644 --- a/support/nfs/nfssvc.c +++ b/support/nfs/nfssvc.cOne wonders why this code is not under utils/nfsd/ . rpc.nfsd seems to be its only caller.Yep...not sure either. I could move it, I suppose.@@ -16,10 +16,9 @@ #include <unistd.h> #include <fcntl.h> #include <errno.h> -#include <syslog.h> - #include "nfslib.h" +#include "xlog.h" #define NFSD_PORTS_FILE "/proc/fs/nfsd/portlist" #define NFSD_VERS_FILE "/proc/fs/nfsd/versions" @@ -57,13 +56,13 @@ nfssvc_setfds(int port, unsigned int ctlbits, char *haddr) if (NFSCTL_UDPISSET(ctlbits)) { udpfd = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP); if (udpfd < 0) { - syslog(LOG_ERR, "nfssvc: unable to create UPD socket: " - "errno %d (%s)\n", errno, strerror(errno)); + xlog(L_ERROR, "unable to create UDP socket: " + "errno %d (%m)", errno); exit(1); } if (bind(udpfd, (struct sockaddr *)&sin, sizeof(sin)) < 0){ - syslog(LOG_ERR, "nfssvc: unable to bind UPD socket: " - "errno %d (%s)\n", errno, strerror(errno)); + xlog(L_ERROR, "unable to bind UDP socket: " + "errno %d (%m)", errno); exit(1); } } @@ -71,32 +70,32 @@ nfssvc_setfds(int port, unsigned int ctlbits, char *haddr) if (NFSCTL_TCPISSET(ctlbits)) { tcpfd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); if (tcpfd < 0) { - syslog(LOG_ERR, "nfssvc: unable to createt tcp socket: " - "errno %d (%s)\n", errno, strerror(errno)); + xlog(L_ERROR, "unable to createt tcp socket: "You could correct the misspelling here (createt), while you're at it.I think I fix that in a later patch, but I'll fix it here too.+ "errno %d (%m)", errno); exit(1); } if (setsockopt(tcpfd, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on)) < 0) { - syslog(LOG_ERR, "nfssvc: unable to set SO_REUSEADDR: " - "errno %d (%s)\n", errno, strerror(errno)); + xlog(L_ERROR, "unable to set SO_REUSEADDR: " + "errno %d (%m)", errno); exit(1); } if (bind(tcpfd, (struct sockaddr *)&sin, sizeof(sin)) < 0){ - syslog(LOG_ERR, "nfssvc: unable to bind TCP socket: " - "errno %d (%s)\n", errno, strerror(errno)); + xlog(L_ERROR, "unable to bind TCP socket: " + "errno %d (%m)", errno); exit(1); } if (listen(tcpfd, 64) < 0){ - syslog(LOG_ERR, "nfssvc: unable to create listening socket: " - "errno %d (%s)\n", errno, strerror(errno)); + xlog(L_ERROR, "unable to create listening socket: " + "errno %d (%m)", errno); exit(1); } } if (udpfd >= 0) { snprintf(buf, BUFSIZ,"%d\n", udpfd); if (write(fd, buf, strlen(buf)) != strlen(buf)) { - syslog(LOG_ERR, - "nfssvc: writing fds to kernel failed: errno %d (%s)", - errno, strerror(errno)); + xlog(L_ERROR, + "writing fds to kernel failed: errno %d (%m)", + errno); } close(fd); fd = -1; @@ -106,9 +105,9 @@ nfssvc_setfds(int port, unsigned int ctlbits, char *haddr) fd = open(NFSD_PORTS_FILE, O_WRONLY); snprintf(buf, BUFSIZ,"%d\n", tcpfd); if (write(fd, buf, strlen(buf)) != strlen(buf)) { - syslog(LOG_ERR, - "nfssvc: writing fds to kernel failed: errno %d (%s)", - errno, strerror(errno)); + xlog(L_ERROR, + "writing fds to kernel failed: errno %d (%m)", + errno); } } close(fd); @@ -139,10 +138,9 @@ nfssvc_versbits(unsigned int ctlbits, int minorvers4) minorvers4 > 0 ? '+' : '-', n); snprintf(ptr+off, BUFSIZ - off, "\n"); - if (write(fd, buf, strlen(buf)) != strlen(buf)) { - syslog(LOG_ERR, "nfssvc: Setting version failed: errno %d (%s)", - errno, strerror(errno)); - } + if (write(fd, buf, strlen(buf)) != strlen(buf)) + xlog(L_ERROR, "Setting version failed: errno %d (%m)", errno); + close(fd); return; diff --git a/utils/nfsd/nfsd.c b/utils/nfsd/nfsd.c index c7bd003..6dfea67 100644 --- a/utils/nfsd/nfsd.c +++ b/utils/nfsd/nfsd.c @@ -18,13 +18,13 @@ #include <string.h> #include <errno.h> #include <getopt.h> -#include <syslog.h> #include <netdb.h> #include <sys/socket.h> #include <netinet/in.h> #include <arpa/inet.h> #include "nfslib.h" +#include "xlog.h" static void usage(const char *); @@ -37,6 +37,7 @@ static struct option longopts[] = { "no-udp", 0, 0, 'U' }, { "port", 1, 0, 'P' }, { "port", 1, 0, 'p' }, + { "debug", 0, 0, 'd' }, { NULL, 0, 0, 0 } }; unsigned int protobits = NFSCTL_ALLBITS; @@ -50,7 +51,7 @@ main(int argc, char **argv) int count = 1, c, error, port, fd, found_one; struct servent *ent; struct hostent *hp; - char *p; + char *p, *progname; ent = getservbyname ("nfs", "udp"); if (ent != NULL) @@ -58,8 +59,22 @@ main(int argc, char **argv) else port = 2049; - while ((c = getopt_long(argc, argv, "H:hN:p:P:TU", longopts, NULL)) != EOF) { + progname = strdup(basename(argv[0])); + if (!progname) { + fprintf(stderr, "%s: unable to allocate memory.\n", argv[0]); + exit(1); + } + + xlog_syslog(1); + xlog_stderr(0); + + while ((c = getopt_long(argc, argv, "dH:hN:p:P:TU", longopts, NULL)) != EOF) { switch(c) { + case 'd': + xlog_config(D_ALL, 1); + xlog_syslog(0); + xlog_stderr(1); + break; case 'H': if (inet_addr(optarg) != INADDR_NONE) { haddr = strdup(optarg); @@ -67,8 +82,8 @@ main(int argc, char **argv) haddr = inet_ntoa((*(struct in_addr*)(hp->h_addr_list[0]))); } else { fprintf(stderr, "%s: Unknown hostname: %s\n", - argv[0], optarg); - usage(argv [0]); + progname, optarg); + usage(progname); } break; case 'P': /* XXX for nfs-server compatibility */ @@ -76,8 +91,8 @@ main(int argc, char **argv) port = atoi(optarg); if (port <= 0 || port > 65535) { fprintf(stderr, "%s: bad port number: %s\n", - argv[0], optarg); - usage(argv [0]); + progname, optarg); + usage(progname); } break; case 'N': @@ -105,14 +120,17 @@ main(int argc, char **argv) default: fprintf(stderr, "Invalid argument: '%c'\n", c); case 'h': - usage(argv[0]); + usage(progname); } } + + xlog_open(progname); + /* * Do some sanity checking, if the ctlbits are set */ if (!NFSCTL_UDPISSET(protobits) && !NFSCTL_TCPISSET(protobits)) { - fprintf(stderr, "invalid protocol specified\n"); + xlog(L_ERROR, "invalid protocol specified");Nit: This code looks like additional command line option parsing, so fprintf(stderr) might be entirely adequate. Any reason to promote these to xlog() ?I think when I asked you about this before you said that you thought that nfsd should always log to syslog whenever possible.
Remember I said that after I mentioned I hadn't looked at this code closely ;-)
Once the command line parsing is done, it's safe to switch to syslog...
Yes, the question is where that point is.
I'm starting to wonder though whether that's the ideal thing. Giventhat rpc.nfsd isn't really a daemon per-se, maybe it would be better tomake it send output to stderr instead? The parent processes for init scripts usually take care of logging stderr to syslog. If someone is running interactively then it probably makes sense to give them feedback on stderr rather than requiring them to run with --debug...
This is one of those cases where different init script behavior across distributions is a challenge. It might be better to provide command- line options to direct the output, rather than a simple "debug" option. Just a thought.
exit(1); } found_one = 0; @@ -121,12 +139,12 @@ main(int argc, char **argv) found_one = 1; } if (!found_one) { - fprintf(stderr, "no version specified\n"); + xlog(L_ERROR, "no version specified"); exit(1); } if (NFSCTL_VERISSET(versbits, 4) && !NFSCTL_TCPISSET(protobits)) { - fprintf(stderr, "version 4 requires the TCP protocol\n"); + xlog(L_ERROR, "version 4 requires the TCP protocol"); exit(1); } if (haddr == NULL) { @@ -135,17 +153,15 @@ main(int argc, char **argv) } if (chdir(NFS_STATEDIR)) { - fprintf(stderr, "%s: chdir(%s) failed: %s\n", - argv [0], NFS_STATEDIR, strerror(errno)); + xlog(L_ERROR, "chdir(%s) failed: %m", NFS_STATEDIR); exit(1); } if (optind < argc) { if ((count = atoi(argv[optind])) < 0) { /* insane # of servers */ - fprintf(stderr, - "%s: invalid server count (%d), using 1\n", - argv[0], count); + xlog(L_ERROR, "invalid server count (%d), using 1", + count); count = 1; } } @@ -155,21 +171,20 @@ main(int argc, char **argv) everything before spawning kernel threads. --Chip */ fd = open("/dev/null", O_RDWR); if (fd == -1) - perror("/dev/null"); + xlog(L_ERROR, "Unable to open /dev/null: %m"); else { (void) dup2(fd, 0); (void) dup2(fd, 1); (void) dup2(fd, 2); } + xlog_syslog(1); + xlog_stderr(0); closeall(3); - openlog("nfsd", LOG_PID, LOG_DAEMON); - if ((error = nfssvc(port, count, versbits, minorvers4, protobits, haddr)) < 0) { - int e = errno; - syslog(LOG_ERR, "nfssvc: %s", strerror(e)); - closelog(); - } + if ((error = nfssvc(port, count, versbits, minorvers4, protobits, haddr)) < 0) + xlog(L_ERROR, "nfssvc (%m)"); + free(progname); return (error != 0); } @@ -177,7 +192,7 @@ static void usage(const char *prog) { fprintf(stderr, "Usage:\n" - "%s [-H hostname] [-p|-P|--port port] [-N|--no-nfs-version version ] [-T|--no-tcp] [-U|--no-udp] nrservs\n", + "%s [-H hostname] [-p|-P|--port port] [-N|--no-nfs-version version ] [-T|--no-tcp] [-U|--no-udp] [-d|--debug] nrservs\n", prog); exit(2); } -- 1.6.2.2-- Chuck Lever chuck[dot]lever[at]oracle[dot]com-- Jeff Layton <jlayton@xxxxxxxxxx>
-- Chuck Lever chuck[dot]lever[at]oracle[dot]com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
