From: Ingo Schwarze Subject: Re: responsive watch wip To: Ted Unangst Cc: tech@openbsd.org Date: Wed, 11 Jun 2025 03:00:06 +0200 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