bsdgrep(1): Revise test case for soon-to-be failure
ClosedPublic
Actions

Authored by kevans on May 2 2017, 4:00 PM.

Details

Reviewers

emaste
cem
ngie

Summary

D10315 is going to make egrep_empty_invalid an actually invalid regex,
to be consistent with the equivalent BRE "{" behavior, when using regex(3).

This test case is ultimately testing for the wrong thing- we're trying to detect
a clean exit, which can be pretty much any non-0 exit value depending on how the
installed grep interprets the expression. gnugrep interprets it as non-matching,
and in the future bsdgrep will interpret it is an error.

Ultimately, we don't care if grep exits with 1 or > 1 since interpretation is
implementation defined, so we use not-exit:0 instead to respect this.

Test Plan

Run kyua tests to ensure no regression, both with bsdgrep as /usr/bin/grep
and gnugrep as /usr/bin/grep

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 9023
Build 9428: arc lint + arc unit

Event Timeline

kevans created this revision.May 2 2017, 4:00 PM

cem added inline comments.May 2 2017, 4:10 PM

usr.bin/grep/grep.c
742–745	This changes program flow if the qflag is present. Is that really correct? Probably qflag should cause us to exit(2) in place of errx(2, ...)?

kevans added inline comments.May 2 2017, 4:15 PM

usr.bin/grep/grep.c
742–745	I think it gets kind of sketchy here, depending on your interpretation of POSIX wording If the -q option is specified, the exit status shall be zero if an input line is selected, even if an error was detected. Otherwise, default actions shall be performed. My interpretation of this is that we should continue to try and match with what patterns we've accepted (if any) if the -q flag is specified. I may need to go back and add some more logic to make sure that this pattern that produced an error isn't used.

kevans added inline comments.May 2 2017, 4:25 PM

usr.bin/grep/grep.c
742–745	Further examination shows that trying to use such a pattern in `regexec(3)` will do the right thing and throw an error, introducing no further problems and leaving me no reason to introduce further logic to avoid using failed patterns.

If it will just error and not possibly match, what's the point of continuing on instead of exiting at the same place as !qflag?

In D10572#219108, @cem wrote:

If it will just error and not possibly match, what's the point of continuing on instead of exiting at the same place as !qflag?

Because we might have other patterns (multiple -e options) that *could* still be valid. Exiting shortly after could be desirable if we find that we actually don't have any valid patterns, though. Especially for large sets of input files.

My previous interpretation of POSIX was wrong; the term 'error' does not extend
to POSIX error and I was using a wrong test case against gnugrep. Back those bits out,
just revise the test to make sure we're exiting cleanly and not matching.

Harbormaster completed remote builds in B9026: Diff 27935.May 2 2017, 5:03 PM

kevans retitled this revision from bsdgrep(1): Revise test case for soon-to-be failure, suppress errors properly to bsdgrep(1): Revise test case for soon-to-be failure.May 2 2017, 5:03 PM

kevans edited the summary of this revision. (Show Details)

In D10572#219129, @kevans91_ksu.edu wrote:

My previous interpretation of POSIX was wrong; the term 'error' does not extend
to POSIX error and I was using a wrong test case against gnugrep. Back those bits out,
just revise the test to make sure we're exiting cleanly and not matching.

What do you mean by POSIX error?

cem accepted this revision.May 2 2017, 5:07 PM

This revision is now accepted and ready to land.May 2 2017, 5:07 PM

ngie added inline comments.May 2 2017, 5:08 PM

contrib/netbsd-tests/usr.bin/grep/t_grep.sh
372–373	0 vs 1 has a distinct meaning with grep, and the above change loosens the exit code check. Per http://pubs.opengroup.org/onlinepubs/9699919799/utilities/grep.html : EXIT STATUS The following exit values shall be returned: 0 One or more lines were selected. 1 No lines were selected. >1 An error occurred. Which behavior are you expecting should be triggered: `>1`?

In D10572#219135, @ngie wrote:

What do you mean by POSIX error?

Uf, s/POSIX/regex/ -- the term 'error' doesn't seem to extend to regex errors after further examination, just other kinds of errors throughout execution.

kevans added inline comments.May 2 2017, 5:16 PM

contrib/netbsd-tests/usr.bin/grep/t_grep.sh
372–373	I'm expecting either: `1` or `>1`. For this particular expression, undefined behavior is abound and "{" could be either an error or some other valid interpretation, but in any case it should not die and at the very least not produce a match against /dev/null.