[src/trunk]: src/usr.bin/make make(1): add a few remarks to JobOutput

[rillig <rillig%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/c31aa43bfeb4
branches:  trunk
changeset: 1017644:c31aa43bfeb4
user:      rillig <rillig%NetBSD.org@localhost>
date:      Sat Jan 02 20:09:06 2021 +0000

description:
make(1): add a few remarks to JobOutput

That function is not used in practice.  Still, there are a lot of subtle
details that can get wrong in that code.

diffstat:

 usr.bin/make/job.c |  39 ++++++++++++++++++++-------------------
 1 files changed, 20 insertions(+), 19 deletions(-)

diffs (78 lines):

diff -r 09354a9af21f -r c31aa43bfeb4 usr.bin/make/job.c
--- a/usr.bin/make/job.c        Sat Jan 02 19:50:42 2021 +0000
+++ b/usr.bin/make/job.c        Sat Jan 02 20:09:06 2021 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: job.c,v 1.391 2020/12/30 10:03:16 rillig Exp $ */
+/*     $NetBSD: job.c,v 1.392 2021/01/02 20:09:06 rillig Exp $ */
 
 /*
  * Copyright (c) 1988, 1989, 1990 The Regents of the University of California.
@@ -143,7 +143,7 @@
 #include "trace.h"
 
 /*     "@(#)job.c      8.2 (Berkeley) 3/19/94" */
-MAKE_RCSID("$NetBSD: job.c,v 1.391 2020/12/30 10:03:16 rillig Exp $");
+MAKE_RCSID("$NetBSD: job.c,v 1.392 2021/01/02 20:09:06 rillig Exp $");
 
 /*
  * A shell defines how the commands are run.  All commands for a target are
@@ -1736,41 +1736,42 @@
 }
 
 /*
- * Print the output of the shell command, skipping the noPrint command of
- * the shell, if any.
+ * Print the output of the shell command, skipping the noPrint text of the
+ * shell, if any.  The default shell does not have noPrint though, which means
+ * that in all practical cases, handling the output is left to the caller.
  */
 static char *
-JobOutput(char *cp, char *endp)
+JobOutput(char *cp, char *endp)        /* XXX: should all be const */
 {
-       char *ecp;
+       char *ecp;              /* XXX: should be const */
 
        if (shell->noPrint == NULL || shell->noPrint[0] == '\0')
                return cp;
 
+       /*
+        * XXX: What happens if shell->noPrint occurs on the boundary of
+        * the buffer?  To work correctly in all cases, this should rather
+        * be a proper stream filter instead of doing string matching on
+        * selected chunks of the output.
+        */
        while ((ecp = strstr(cp, shell->noPrint)) != NULL) {
                if (ecp != cp) {
-                       *ecp = '\0';
+                       *ecp = '\0';    /* XXX: avoid writing to the buffer */
                        /*
                         * The only way there wouldn't be a newline after
                         * this line is if it were the last in the buffer.
-                        * however, since the non-printable comes after it,
+                        * however, since the noPrint output comes after it,
                         * there must be a newline, so we don't print one.
                         */
+                       /* XXX: What about null bytes in the output? */
                        (void)fprintf(stdout, "%s", cp);
                        (void)fflush(stdout);
                }
                cp = ecp + shell->noPrintLen;
-               if (cp != endp) {
-                       /*
-                        * Still more to print, look again after skipping
-                        * the whitespace following the non-printable
-                        * command.
-                        */
-                       cp++;
-                       pp_skip_whitespace(&cp);
-               } else {
-                       return cp;
-               }
+               if (cp == endp)
+                       break;
+               cp++;           /* skip over the (XXX: assumed) newline */
+               pp_skip_whitespace(&cp);
        }
        return cp;
 }