Re: [PATCH 03/10] nfs-utils: convert rpc.nfsd to use xlog()

[Steve Dickson <SteveD@redhat.com>]

Jeff Layton wrote:
> On Wed, 3 Jun 2009 16:01:24 -0400
> Chuck Lever <chuck.lever@oracle.com> 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 that
>>> makes log messages go to stderr. Eventually however nfsd has to close
>>> stderr, 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@redhat.com>
>>> ---
>>> 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.c
>>> @@ -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: "
>>> +				"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");
>>> 		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)");
>> Nit: Maybe get rid of the "nfssvc" here as well?
Hmm... I would think this needs to be a 
    xlog(LOG_ERR, "nfssvc: errno %d (%s)", e, strerror(e));

>>
> 
> Well, the "nfssvc" here tells us that the nfssvc() call returned an
> error. Later patches will clean that up when I break up the nfssvc()
> interface. That said though, there's a bug there. %m isn't appropriate
> since errno isn't being set. Whoops!
> 
>>>
>>> +	free(progname);
>> You go to the trouble of freeing progname here, but you exit in some  
>> cases in the code above without freeing.  I usually copy argv[0] into  
>> a static buffer to avoid all that strdup(3) bother.
>>
> 
> Six of one, half dozen of another. I try to be good about freeing
> memory that's been allocated in case this code gets reorganized in the
> future. The existing code is pretty bad about it, but it doesn't
> matter much since this program isn't long running.
> 
> As far as allocating a static buffer for it, the problem there is that
> you don't know how big a buffer you'll need. Using strdup means less
> memory is being used.
Since this is not a long standing daemon (i.e. it exits) and as 
long as the free() does cause a problem.. I don't see a problem with it..


steved.