Re: tee: Replace hand-rolled list with SLIST_*

[Klemens Nanni]

On Mon, Jul 10, 2017 at 07:49:10PM +0200, Alexander Bluhm wrote:
> 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.
Points taken, thanks for your review/feedback!

> Just do 1, then chances are higher that it gets commited.
Here's the updated diff handling the list->SLIST conversion alone.

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       10 Jul 2017 18:41:46 -0000
@@ -32,6 +32,7 @@
 
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <sys/queue.h>
 
 #include <err.h>
 #include <errno.h>
@@ -43,11 +44,11 @@
 #include <unistd.h>
 
 struct list {
-       struct list *next;
+       SLIST_ENTRY(list) next;
        int fd;
        char *name;
 };
-struct list *head;
+SLIST_HEAD(, list) head;
 
 static void
 add(int fd, char *name)
@@ -58,8 +59,7 @@ add(int fd, char *name)
                err(1, NULL);
        p->fd = fd;
        p->name = name;
-       p->next = head;
-       head = p;
+       SLIST_INSERT_HEAD(&head, p, next);
 }
 
 int
@@ -75,6 +75,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,7 +111,7 @@ 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(p, &head, next) {
                        n = rval;
                        bp = buf;
                        do {
@@ -127,7 +129,7 @@ main(int argc, char *argv[])
                exitval = 1;
        }
 
-       for (p = head; p; p = p->next) {
+       SLIST_FOREACH(p, &head, next) {
                if (close(p->fd) == -1) {
                        warn("%s", p->name);
                        exitval = 1;