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
> > > > > <yaneg...@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.
[...]
> 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));
> + } else if (!S_ISDIR(sb.st_mode))
You don't need { } here.
> + 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);
You override 'fd'. After first file read it will stop working.
> + if (fd < 0 || fstat(fd, &sb) != 0) {
> + warn("%s", de->d_name);
If 'fd' is >= 0 you should close it.
> + continue;
> + } else if (!S_ISREG(sb.st_mode))
> + continue;
You are leaking it here as well.
> + warncount += parsefile(fd);
> + close(fd);
[...]
> - fclose(file);
Removing fclose() is wrong. Once you do fdopen(3) you should use
fclose(), instead you removed fclose(3) and you close(2) after
parsefile() returned. You leak FILE structure this way.
--
Pawel Jakub Dawidek http://www.wheelsystems.com
FreeBSD committer http://www.FreeBSD.org
Am I Evil? Yes, I Am! http://tupytaj.pl
pgpx0ttpn6kwi.pgp
Description: PGP signature
