Date: Sun, 20 Jul 2008 17:14:07 -0700 From: "Garrett Cooper" <yanefbsd@gmail.com> To: "Antoine Brodin" <antoine@freebsd.org> Cc: freebsd-bugs@freebsd.org Subject: Re: bin/125680: atacontrol(8): atacontrol depends on executable in /usr Message-ID: <7d6fde3d0807201714g49eb4a80ncfcc1cc800ad595e@mail.gmail.com> In-Reply-To: <200807202210.m6KMA4cm032331@freefall.freebsd.org> References: <200807202210.m6KMA4cm032331@freefall.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Jul 20, 2008 at 3:10 PM, Antoine Brodin <antoine@freebsd.org> wrote:
> The following reply was made to PR bin/125680; it has been noted by GNATS.
>
> From: Antoine Brodin <antoine@FreeBSD.org>
> To: bug-followup@FreeBSD.org, stef@memberwebs.com
> Cc:
> Subject: Re: bin/125680: atacontrol(8): atacontrol depends on executable in
> /usr
> Date: Sun, 20 Jul 2008 23:37:13 +0200
>
> This is a multi-part message in MIME format.
>
> --Multipart=_Sun__20_Jul_2008_23_37_13_+0200_SBQJ0Ag63L4yRB6c
> Content-Type: text/plain; charset=US-ASCII
> Content-Transfer-Encoding: 7bit
>
> Could you try the attached patch ?
>
> Cheers,
>
> Antoine
>
> --Multipart=_Sun__20_Jul_2008_23_37_13_+0200_SBQJ0Ag63L4yRB6c
> Content-Type: text/x-diff;
> name="atacontrol.diff"
> Content-Disposition: attachment;
> filename="atacontrol.diff"
> Content-Transfer-Encoding: 7bit
>
> Index: sbin/atacontrol/atacontrol.c
> ===================================================================
> RCS file: /home/ncvs/src/sbin/atacontrol/atacontrol.c,v
> retrieving revision 1.48
> diff -u -r1.48 atacontrol.c
> --- sbin/atacontrol/atacontrol.c 15 May 2008 01:25:29 -0000 1.48
> +++ sbin/atacontrol/atacontrol.c 20 Jul 2008 21:18:35 -0000
> @@ -37,6 +37,7 @@
> #include <stdlib.h>
> #include <string.h>
> #include <sysexits.h>
> +#include <unistd.h>
>
> static const char *
> mode2str(int mode)
> @@ -517,12 +518,29 @@
> if (ioctl(fd, IOCATARAIDREBUILD, &array) < 0)
> warn("ioctl(IOCATARAIDREBUILD)");
> else {
> - char buffer[128];
> - sprintf(buffer, "/usr/bin/nice -n 20 /bin/dd "
> - "if=/dev/ar%d of=/dev/null bs=1m &",
> - array);
> - if (system(buffer))
> - warn("background dd");
> + char device[64];
> + char *buffer;
> + int arfd;
> +
> + switch (fork()) {
> + case -1:
> + err(1, "fork");
> + case 0:
> + nice(20);
> + snprintf(device, sizeof(device), "/dev/ar%d",
> + array);
> + if ((arfd = open(device, O_RDONLY)) == -1)
> + err(1, "open %s", device);
> + if ((buffer = malloc(1024 * 1024)) == NULL)
> + err(1, "malloc");
> + while (read(arfd, buffer, 1024 * 1024) > 0)
> + ;
> + free(buffer);
> + close(arfd);
> + break;
> + default:
> + break;
> + }
> }
> exit(EX_OK);
> }
Good catch. C apps shouldn't depend on other commands' existence for
security and performance reasons.
A few comments:
1. Why is forking another process necessary? In your patch above
you're forcing the parent to do the work while the child goes and
dies, so there really isn't any benefit to forking at all other than
just exercise fork a bit.
2. If you're going to fork another process, wouldn't it be wiser to
waitpid(2) until the child's done?
Thanks,
-Garrett
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?7d6fde3d0807201714g49eb4a80ncfcc1cc800ad595e>