Hello Ted,

Ted Unangst wrote on Mon, Jun 09, 2025 at 11:28:35PM -0400:

> The read_result function blocks until the child exits. This makes
> watch unresponsive. Try it with a short shell script:
>     echo hello; sleep 5; echo goodbye
> 
> You can't even quit by pressing q because it's stuck reading lines.

That feels like a valid concern.

I see a significantly more serious issue though.  Maybe we should
look into that once you have sorted your issue out and some form
of your improvements is in.  Sorry if this sounds like hijacking
the thread, that's not my intention.  I'm bringing it up now because
it is somewhat related, and definitely related to some of your
comments in your original mail.

   $ locale charmap
  UTF-8
   $ watch printf \'first line\\nprefix \\xff suffix\\n\'
  Every 1s: printf 'first line\nprefix \xff...   isnote.usta.de 02:18:56
  
  first line
  prefix

The output is incomplete, and there is no indication that there is
any problem.  That's due to the fact that the current code calls
fgetws(3).  That function is a horrific abomination that is almost
never adequate for any purpose:

  RETURN VALUES
     [...]
     If an error occurs, fgetws() returns NULL and the buffer
     contents are indeterminate.

Even if the programmer is OK with stopping parsing on the first
error - which very rarely makes sense, and certainly not for this
utility - that function is almost always still unusable because
adequate error reporting is completely impossible.  The caller
can neither report to the user where exactly the input error
occurred, nor reparse the input with better functions to find
more details about the error because the input is already gone
from the input stream and buffer.

I'm appending a patch at the end that is *not* intended for
commit, but only intended to demonstrate:

 1) how difficult it is to use fgetws(3) correctly
 2) and that, even if it uses fgetws(3) correctly, the program
    can only do error reporting that is utterly unhelpful.

Instead of this patch, the code needs to be rewritten without using
fgetws(3).  There are multiple ways for doing that.  One very robust
way is with getline(3) and mbtowc(3) - but not with mbrtowc(3),
please - with the downside that it may block if the watched program
causes long delays in the middle of a line of output (consider echo -n
foo; sleep 10; echo bar).  Since using fgetwc(3) is *not* possible -
the POSIX standard explicitly says that the position of the file
pointer becomes undefined on error - a good solution probably needs
a read(2) loop combined with mbrtowc(3), or a fgetc(3) loop combined
with mbrtowc(3).

> This diff rearranges the whole fork and read pipeline to use libevent.
> Now user commands and input can be interleaved with the command. You
> can press q at any time, partial updates are displayed, etc.
> 
> I left all the old code intact for reference, because there's more work
> to do. There's still a bunch of calls to read_result.
> 
> Needs mbstowc in the child_input function.

There is no such thing as mbstowc.  I don't see mbstowcs(3) in your
diffs, but that is probably a good thing.  That function is another
abomination that is only marginally better than fgetws(3), in the
following sense: fgetws(3) makes good error reporting completely
impossible.  mbstowcs(3) does not make good error reporting
impossible, but so difficult that it's much simpler to not use
mbstowcs(3) in the first place.

Yours,
  Ingo


Index: watch.c
===================================================================
RCS file: /cvs/src/usr.bin/watch/watch.c,v
diff -u -p -r1.28 watch.c
--- watch.c	31 May 2025 08:26:26 -0000	1.28
+++ watch.c	11 Jun 2025 00:16:57 -0000
@@ -382,7 +382,7 @@ void
 read_result(BUFFER *buf)
 {
 	FILE	*fp;
-	int	 i, st, fds[2];
+	int	 i, st, fds[2], read_errno;
 	pid_t	 pipe_pid, pid;
 
 	/* Clear buffer */
@@ -414,10 +414,14 @@ read_result(BUFFER *buf)
 		err(1, "fdopen()");
 	close(fds[1]);
 
+	clearerr(fp);
+	read_errno = 0;
 	/* Read command output and convert tab to spaces * */
 	for (i = 0; i < MAXLINE && fgetws((*buf)[i], MAXCOLUMN, fp) != NULL;
 	    i++)
 		untabify((*buf)[i], sizeof((*buf)[i]));
+	if (ferror(fp))
+		read_errno = errno;
 	fclose(fp);
 	do {
 		pid = waitpid(pipe_pid, &st, 0);
@@ -430,6 +434,8 @@ read_result(BUFFER *buf)
 		last_exitcode = WEXITSTATUS(st);
 	if (pause_on_error && last_exitcode)
 		paused = 1;
+	if (read_errno)
+		(void)mbstowcs((*buf)[i], strerror(read_errno), MAXCOLUMN);
 }
 
 kbd_result_t