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;
