Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 21 Dec 2012 17:58:42 +0100
From:      Pawel Jakub Dawidek <pjd@FreeBSD.org>
To:        Mark Johnston <markjdb@gmail.com>
Cc:        Ian Lepore <freebsd@damnhippie.dyndns.org>, Brooks Davis <brooks@FreeBSD.org>, d@delphij.net, Jilles Tjoelker <jilles@stack.nl>, Garrett Cooper <yanegomi@gmail.com>, "svn-src-all@freebsd.org" <svn-src-all@FreeBSD.org>, Alfred Perlstein <bright@mu.org>, "src-committers@freebsd.org" <src-committers@FreeBSD.org>, Xin LI <delphij@FreeBSD.org>, "svn-src-head@freebsd.org" <svn-src-head@FreeBSD.org>, Andrey Zonov <zont@FreeBSD.org>
Subject:   Re: svn commit: r244198 - in head: etc/rc.d sbin/sysctl
Message-ID:  <20121221165842.GE1397@garage.freebsd.pl>
In-Reply-To: <20121219235917.GD8399@oddish>
References:  <1355932607.1198.206.camel@revolution.hippie.lan> <F158CD10-B65B-4BBA-BCAD-6A52BC5C8FDF@mu.org> <50D2128A.7030205@delphij.net> <20121219210418.GA83983@stack.nl> <CAGH67wRTdBCZLq1R8-yqD6EBZ%2B=F228-0o_8TvU3b0KZD6vUfg@mail.gmail.com> <CAGH67wT1L_9ia7-hdHST6q8iCdtmFifZcHixCQhBTiZEH8YAcw@mail.gmail.com> <50D23961.7090803@delphij.net> <20121219225854.GA8399@oddish> <20121219232140.GA40927@lor.one-eyed-alien.net> <20121219235917.GD8399@oddish>

next in thread | previous in thread | raw e-mail | index | archive | help

--JBi0ZxuS5uaEhkUZ
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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
> > > >=20
> > > > On 12/19/12 13:12, Garrett Cooper wrote:
> > > > > On Wed, Dec 19, 2012 at 1:10 PM, Garrett Cooper
> > > > > <yanegomi@gmail.com> wrote:
> > > > >=20
> > > > > ...
> > > > >=20
> > > > >> find -exec / echo | xargs ? Seems like there's a better way to
> > > > >> solve this.
> > > > >=20
> > > > > Of course we also might be overengineering the problem (my=20
> > > > > suggestion definitely was overengineered). Why not pass in the=20
> > > > > appropriate arguments via sysctl_args in /etc/rc.conf ? Thanks,
> > > >=20
> > > > 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 wro=
ng,
> > > > they just remove the pack rather than changing /etc/rc.conf.
> > >=20
> > > 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 w=
ay
> > > we can have a directory, say /etc/syslog.d, into which package install
> > > scripts can easily drop config files.
> > >=20
> > > 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.
> >=20
> > I don't have a strong opinion about the usefulness of this feature.  It
> > seems useful particularly in conjunction with supporting multiple -f's.
>=20
> I don't really either. Just thought I'd suggest it.
>=20
> >=20
> > I do have a few comments on the implementation below.
> >=20
>=20
> 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 =3D 0;
> +
> +	fd =3D 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 =3D fdopendir(fd);
> +	if (dir =3D=3D NULL)
> +		err(EX_NOINPUT, "%s", filename);
> +	while ((de =3D readdir(dir)) !=3D NULL) {
> +		if (fnmatch("*.conf", de->d_name, 0) !=3D 0)
> +			continue;
> +		fd =3D openat(fd, de->d_name, O_RDONLY);

You override 'fd'. After first file read it will stop working.

> +		if (fd < 0 || fstat(fd, &sb) !=3D 0) {
> +			warn("%s", de->d_name);

If 'fd' is >=3D 0 you should close it.

> +			continue;
> +		} else if (!S_ISREG(sb.st_mode))
> +			continue;

You are leaking it here as well.

> +		warncount +=3D 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.

--=20
Pawel Jakub Dawidek                       http://www.wheelsystems.com
FreeBSD committer                         http://www.FreeBSD.org
Am I Evil? Yes, I Am!                     http://tupytaj.pl

--JBi0ZxuS5uaEhkUZ
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (FreeBSD)

iEYEARECAAYFAlDUlUIACgkQForvXbEpPzRZdACfY2Jiga1LFREBbgtlJo7XlhDp
t70AnjkOIcY5oj8Wboh40LEzsqZxCLCs
=io2b
-----END PGP SIGNATURE-----

--JBi0ZxuS5uaEhkUZ--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121221165842.GE1397>