On Sun, Jul 02, 2017 at 04:29:39AM +0200, Klemens Nanni wrote:
> No functional change or bugfix but queue(3) is the for a reason. That
> way the code even is a tad clearer to me (and two lines shorter).
>
> Feedback/OK?
You do too much in this diff:
1. convert list to SLIST
2. make fp a global variable
3. rename p to fp
4. rename struct list to struct file
5. replace malloc(sizeof(*p)) with malloc(sizeof(struct file))
6. Remove braces from last SLIST_FOREACH loop
1. is good.
2. is bad. Local variables are almost always better and easier
to maintain. Using global loop variables is especially error
prone.
3.-6. is just personal taste. Unless you more or less own a source
file, do not touch it. If everyone would change everything to
his personal taste, our history would be full of useles back and
forth.
Just do 1, then chances are higher that it gets commited.
bluhm
>
> Index: tee.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/tee/tee.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 tee.c
> --- tee.c 28 Oct 2016 07:22:59 -0000 1.11
> +++ tee.c 2 Jul 2017 02:14:54 -0000
> @@ -32,6 +32,7 @@
>
> #include <sys/types.h>
> #include <sys/stat.h>
> +#include <sys/queue.h>
>
> #include <err.h>
> #include <errno.h>
> @@ -42,30 +43,26 @@
> #include <string.h>
> #include <unistd.h>
>
> -struct list {
> - struct list *next;
> +struct file {
> + SLIST_ENTRY(file) next;
> int fd;
> char *name;
> -};
> -struct list *head;
> +} *fp;
> +SLIST_HEAD(, file) head;
>
> static void
> add(int fd, char *name)
> {
> - struct list *p;
> -
> - if ((p = malloc(sizeof(*p))) == NULL)
> + if ((fp = malloc(sizeof(struct file))) == NULL)
> err(1, NULL);
> - p->fd = fd;
> - p->name = name;
> - p->next = head;
> - head = p;
> + fp->fd = fd;
> + fp->name = name;
> + SLIST_INSERT_HEAD(&head, fp, next);
> }
>
> int
> main(int argc, char *argv[])
> {
> - struct list *p;
> int fd;
> ssize_t n, rval, wval;
> char *bp;
> @@ -75,6 +72,8 @@ main(int argc, char *argv[])
> if (pledge("stdio wpath cpath", NULL) == -1)
> err(1, "pledge");
>
> + SLIST_INIT(&head);
> +
> append = 0;
> while ((ch = getopt(argc, argv, "ai")) != -1) {
> switch(ch) {
> @@ -109,12 +108,12 @@ main(int argc, char *argv[])
> err(1, "pledge");
>
> while ((rval = read(STDIN_FILENO, buf, sizeof(buf))) > 0) {
> - for (p = head; p; p = p->next) {
> + SLIST_FOREACH(fp, &head, next) {
> n = rval;
> bp = buf;
> do {
> - if ((wval = write(p->fd, bp, n)) == -1) {
> - warn("%s", p->name);
> + if ((wval = write(fp->fd, bp, n)) == -1) {
> + warn("%s", fp->name);
> exitval = 1;
> break;
> }
> @@ -127,12 +126,11 @@ main(int argc, char *argv[])
> exitval = 1;
> }
>
> - for (p = head; p; p = p->next) {
> - if (close(p->fd) == -1) {
> - warn("%s", p->name);
> + SLIST_FOREACH(fp, &head, next)
> + if (close(fp->fd) == -1) {
> + warn("%s", fp->name);
> exitval = 1;
> }
> - }
>
> return exitval;
> }
