Input preprocessor failures are not diagnosed

[eggert]

Currently if the input preprocessor fails, less does not report this failure to the user. This causes problems with applications like Gzip's zless program, which runs less with LESSOPEN='||-gzip -cdfq -- %s' in the environment, and if gzip fails no failure indication is given to the user. I found this out a while ago via Jaroslaw Weglinski's gzip bug report "zless error under ulimit -v" but haven't gotten around to trying to fix it until now.

As I mentioned in that bug report, you can reproduce the problem by creating a large text file t and compressing it with gzip (say, by seq 1000000 >t; gzip t), and then by running the following shell command in one window:

LESSOPEN='||-gzip -cdfq -- %s' less t.gz

When you see the first screenful, go to another window and kill the gzip process that's waiting for less to read more input. less won't notice or report the gzip failure.

Attached is a proposed patch to less that will cause it to diagnose input preprocessor failures like this. The idea is simple: if pclose returns a nonzero value, output an "Input preprocessor failed" diagnostic. Since this means the cleanup edit((char*)NULL); can output a diagnostic, this cleanup call needs to be before quit's call to deinit. I'm by no means a less expert so perhaps you can think of a better fix.

less-fix.txt

[gwsw]

If I apply this patch, run your gzip test and then just type q to exit less (without killing the gzip process), I see the "Input preprocessor failed" message. This is on a Linux 5.12 system. It looks like pclose is returning 512, which I think means that gzip is returning an exit status of 2, which according to the gzip man page means "a warning occurred". I'm not sure exactly why that's happening, but it makes me think there is some risk of false error messages in this change.

[gwsw]

A little more experimentation indicates that gzip returns 2 if its output is not fully consumed:

$ gzip -cdfq -- obj/t.gz | head -3
1
2
3
$ echo ${PIPESTATUS[0]}
2

[eggert]

A little more experimentation indicates that gzip returns 2 if its output is not fully consumed:

Thanks for pointing that out. To avoid that problem, 'less' should check the preprocessor's exit status only if 'less' has fully consumed the preprocessor output. Here is a revised patch. 0001-Diagnose-input-preprocessor-failures-258.patch.txt

[gwsw]

I still have some concerns about making a change which will cause fully functional LESSOPEN scripts to start emitting error messages. Perhaps this should only be applied to scripts invoked with a double vertical bar, since such scripts already need to pay attention to their exit status when the file is empty, so are perhaps more likely to use a zero exit status in other cases too. However, nothing currently depends on the exit status when the file is nonempty, so this could still produce spurious error messages. Perhaps a new LESSOPEN syntax could be introduced, and this new error checking would apply only if LESSOPEN begins with THREE vertical bars (although I hesitate to suggest such ugliness).

[eggert]

Perhaps this should only be applied to scripts invoked with a double vertical bar

Yes, that's what the revised patch does.

nothing currently depends on the exit status when the file is nonempty, so this could still produce spurious error messages

Can you give a practical example where the error messages would be spurious? I can't think of one.

Perhaps a new LESSOPEN syntax could be introduced, and this new error checking would apply only if LESSOPEN begins with THREE vertical bars

I can easily generate a new patch to implement that, although I hope this isn't necessary.

[gwsw]

Well, it's entirely determined by the LESSOPEN script that the user is using, which is outside my control, and may be poorly written but still functional. For example, a script might decompress its input and then call "exit 1" for whatever reason, and today it would run fine, even when invoked with two bars, as long as the input is not empty.

[eggert]

a script might decompress its input and then call "exit 1"

I think it unlikely that a script would do that so I doubt whether this is a practical problem. However, you're correct that it is an incompatibility in theory at least. So, here is a revised patch that implements your suggestion of using the proposed behavior only if there are three vertical bars. This should allay any compatibility concerns.

0001-Diagnose-input-preprocessor-failures-258.patch.txt

[eggert]

I created PR #315 which should solve the problem, as mentioned above.

[gwsw]

I've integrated your pull request, and made some further changes. After reconsideration, I decided to make this behavior controlled by a command line option (--show-preproc-error) rather than the triple-pipe syntax. Also, since the behavior is now explicitly under the user's control, I decided to let it detect errors even if the pipe is not fully consumed, since there may be cases where the user wants that behavior.

However I just realized that there may be a problem in the logic, because it's checking for the error only when you quit less, and only on the current file. If you view a file with a preprocessor which returns an error, and then switch to viewing a different file and quit, the error won't be detected. That seems incorrect at first glance, but it's not completely clear to me what the right behavior should be in that situation.

[eggert]

For zless (from gzip) this is not a big deal since it reads only standard input; there is no other file.

For other applications it could be an issue. What does 'less' do with I/O errors when it's reading other files ('read' returns -1 with errno == EIO, say)? Perhaps whatever mechanism 'less' uses to report I/O errors could be used to report input preprocessor failures.

[gwsw]

Well, even in zless, the user can type ":e" and view another file.
I guess this isn't really a problem though. If you edit a new file, close_file will call close_altpipe and display the warning message at that point.