svn commit: r244198 - in head: etc/rc.d sbin/sysctl

[Brooks Davis]

On Wed, Dec 19, 2012 at 06:59:17PM -0500, Mark Johnston wrote:
> On Wed, Dec 19, 2012 at 05:21:40PM -0600, Brooks Davis wrote:
> > On Wed, Dec 19, 2012 at 05:58:54PM -0500, Mark Johnston wrote:
> > > On Wed, Dec 19, 2012 at 02:02:09PM -0800, Xin Li wrote:
> > > > -----BEGIN PGP SIGNED MESSAGE-----
> > > > Hash: SHA256
> > > > 
> > > > On 12/19/12 13:12, Garrett Cooper wrote:
> > > > > On Wed, Dec 19, 2012 at 1:10 PM, Garrett Cooper
> > > > > <yanegomi at gmail.com> wrote:
> > > > > 
> > > > > ...
> > > > > 
> > > > >> find -exec / echo | xargs ? Seems like there's a better way to
> > > > >> solve this.
> > > > > 
> > > > > Of course we also might be overengineering the problem (my 
> > > > > suggestion definitely was overengineered). Why not pass in the 
> > > > > appropriate arguments via sysctl_args in /etc/rc.conf ? Thanks,
> > > > 
> > > > Irrelevant.  Consider this (extreme) situation: someone distributes
> > > > several sets of sysctl values tuned for certain situations, like
> > > > tcp.conf, supermicro.conf, ... and wants to put them together in a
> > > > directory, it's useful to source from the directory without having to
> > > > do a generation of command line on boot, so when something goes wrong,
> > > > they just remove the pack rather than changing /etc/rc.conf.
> > > 
> > > At work I've changed the -f flag of syslogd and newsyslog to accept a
> > > directory which gets non-recursively searched for input files. This way
> > > we can have a directory, say /etc/syslog.d, into which package install
> > > scripts can easily drop config files.
> > > 
> > > For something like sysctl this might be a bit much, but it's just a
> > > thought. The example diff below is what I have in mind.
> > 
> > I don't have a strong opinion about the usefulness of this feature.  It
> > seems useful particularly in conjunction with supporting multiple -f's.
> 
> I don't really either. Just thought I'd suggest it.
> 
> > 
> > I do have a few comments on the implementation below.
> > 
> 
> Thanks! I didn't know about openat(). Here's the regenerated diff.
> 
> diff --git a/sbin/sysctl/sysctl.c b/sbin/sysctl/sysctl.c
> index 8fad089..9e453f6 100644
> --- a/sbin/sysctl/sysctl.c
> +++ b/sbin/sysctl/sysctl.c
> @@ -42,6 +42,7 @@ static const char rcsid[] =
>  #endif /* not lint */
>  
>  #include <sys/param.h>
> +#include <sys/types.h>
>  #include <sys/time.h>
>  #include <sys/resource.h>
>  #include <sys/stat.h>
> @@ -49,8 +50,11 @@ static const char rcsid[] =
>  #include <sys/vmmeter.h>
>  
>  #include <ctype.h>
> +#include <dirent.h>
>  #include <err.h>
>  #include <errno.h>
> +#include <fcntl.h>
> +#include <fnmatch.h>
>  #include <inttypes.h>
>  #include <locale.h>
>  #include <stdio.h>
> @@ -64,8 +68,9 @@ static const char *conffile;
>  static int	aflag, bflag, dflag, eflag, hflag, iflag;
>  static int	Nflag, nflag, oflag, qflag, Tflag, Wflag, xflag;
>  
> +static int	handlefile(const char *);
>  static int	oidfmt(int *, int, char *, u_int *);
> -static int	parsefile(const char *);
> +static int	parsefile(int);
>  static int	parse(const char *, int);
>  static int	show_var(int *, int);
>  static int	sysctl_all(int *oid, int len);
> @@ -165,7 +170,7 @@ main(int argc, char **argv)
>  
>  	warncount = 0;
>  	if (conffile != NULL)
> -		warncount += parsefile(conffile);
> +		warncount += handlefile(conffile);
>  
>  	while (argc-- > 0)
>  		warncount += parse(*argv++, 0);
> @@ -402,15 +407,55 @@ parse(const char *string, int lineno)
>  }
>  
>  static int
> -parsefile(const char *filename)
> +handlefile(const char *filename)
> +{
> +	DIR *dir;
> +	struct dirent *de;
> +	struct stat sb;
> +	int fd, warncount = 0;
> +
> +	fd = open(filename, O_RDONLY);
> +	if (fd < 0)
> +		err(EX_NOINPUT, "%s", filename);
> +	if (fstat(fd, &sb))
> +		err(EX_NOINPUT, "%s", filename);
> +
> +	if (S_ISREG(sb.st_mode)) {
> +		return (parsefile(fd));

This leaks the fd.  I might just keep the close() in parsefile() so that
in consumes the fd.

-- Brooks

> +	} else if (!S_ISDIR(sb.st_mode))
> +		errx(EX_USAGE, "invalid input file '%s'", filename);
> +
> +	dir = fdopendir(fd);
> +	if (dir == NULL)
> +		err(EX_NOINPUT, "%s", filename);
> +	while ((de = readdir(dir)) != NULL) {
> +		if (fnmatch("*.conf", de->d_name, 0) != 0)
> +			continue;
> +		fd = openat(fd, de->d_name, O_RDONLY);
> +		if (fd < 0 || fstat(fd, &sb) != 0) {
> +			warn("%s", de->d_name);
> +			continue;
> +		} else if (!S_ISREG(sb.st_mode))
> +			continue;
> +
> +		warncount += parsefile(fd);
> +		close(fd);
> +	}
> +	closedir(dir);
> +
> +	return (warncount);
> +}
> +
> +static int
> +parsefile(int fd)
>  {
>  	FILE *file;
>  	char line[BUFSIZ], *p, *pq, *pdq;
>  	int warncount = 0, lineno = 0;
>  
> -	file = fopen(filename, "r");
> +	file = fdopen(fd, "r");
>  	if (file == NULL)
> -		err(EX_NOINPUT, "%s", filename);
> +		err(EX_OSERR, "fdopen()");
>  	while (fgets(line, sizeof(line), file) != NULL) {
>  		lineno++;
>  		p = line;
> @@ -446,7 +491,6 @@ parsefile(const char *filename)
>  		else
>  			warncount += parse(p, lineno);
>  	}
> -	fclose(file);
>  
>  	return (warncount);
>  }
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 188 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20121219/3d56dd2a/attachment-0001.sig>