I am really against the idea of the parser inspecting a static buffer
from the lex.
Also we have a ton of these parsers, and discourage them from deviating.
This tiny little "please use the right keyword" change feels so minor; we
do not have a generic error-correction-proposing parser, 99% of plausible
errors emit "syntax error".
The only reason "no" is a TOKEN is because of previous use in other
parts of the grammer, whereas "yes" does not occur in other places in
the grammer. Let's keep this simple.
David Gwynne <da...@gwynne.id.au> wrote:
> On Tue, Aug 31, 2021 at 07:33:40AM +0200, Alexandr Nedvedicky wrote:
> > Hello,
> >
> > On Tue, Aug 31, 2021 at 02:40:57PM +1000, David Gwynne wrote:
> > > handling the "no" option with a token, and "yes" via a string made my
> > > eye twitch.
> > >
> > > ok? or is the helpful yyerror a nice feature?
> > >
> >
> > I actually think it's a nice feature. below is output
> > for current pfctl we have in tree:
>
> it is nice, but the implementation isn't... rigorous.
>
> >
> > lumpy$ pfctl -n -f /tmp/pf.conf
> > /tmp/pf.conf:6: invalid value 'nope', expected 'yes' or 'no'
> >
> > and output with diff applied:
> >
> > lumpy$ ./pfctl -n -f /tmp/pf.conf
> > /tmp/pf.conf:6: syntax error
>
> but if you try to use a keyword instead of a string, you get this:
>
> dlg@kbuild ~$ echo "set reassemble yes" | pfctl -vnf -
> set reassemble yes
> dlg@kbuild ~$ echo "set reassemble no" | pfctl -vnf -
> set reassemble no
> dlg@kbuild ~$ echo "set reassemble nope" | pfctl -vnf -
> stdin:1: invalid value 'nope', expected 'yes' or 'no'
> dlg@kbuild ~$ echo "set reassemble block" | pfctl -vnf -
> stdin:1: syntax error
> dlg@kbuild ~$
>
> if the tokeniser exposed the buffer it was working on, we could make it
> consistent for all arguments:
>
> dlg@kbuild pfctl$ echo "set reassemble yes" | ./obj/pfctl -vnf -
> set reassemble yes
> dlg@kbuild pfctl$ echo "set reassemble no" | ./obj/pfctl -vnf -
> set reassemble no
> dlg@kbuild pfctl$ echo "set reassemble nope" | ./obj/pfctl -vnf -
> stdin:1: syntax error
> stdin:1: invalid value 'nope', expected 'yes' or 'no'
> dlg@kbuild pfctl$ echo "set reassemble block" | ./obj/pfctl -vnf -
> stdin:1: syntax error
> stdin:1: invalid value 'block', expected 'yes' or 'no'
>
> the extremely rough PoC diff for pfctl that implements this is
> below. because the tokeniser handles some operators without using the
> buffer, if you give "set reassemble" an operator then you get confusing
> output:
>
> dlg@kbuild pfctl$ echo "set reassemble <" | ./obj/pfctl -vnf -
> stdin:1: syntax error
> stdin:1: invalid value 'reassemble', expected 'yes' or 'no'
>
> anyway, it might be easier to drop the diff for now.
>
> Index: parse.y
> ===================================================================
> RCS file: /cvs/src/sbin/pfctl/parse.y,v
> retrieving revision 1.709
> diff -u -p -r1.709 parse.y
> --- parse.y 1 Feb 2021 00:31:04 -0000 1.709
> +++ parse.y 31 Aug 2021 09:20:38 -0000
> @@ -458,6 +458,8 @@ typedef struct {
> int lineno;
> } YYSTYPE;
>
> +static u_char *yytext;
> +
> #define PPORT_RANGE 1
> #define PPORT_STAR 2
> int parseport(char *, struct range *r, int);
> @@ -471,7 +473,7 @@ int parseport(char *, struct range *r, i
> %token PASS BLOCK MATCH SCRUB RETURN IN OS OUT LOG QUICK ON FROM TO
> FLAGS
> %token RETURNRST RETURNICMP RETURNICMP6 PROTO INET INET6 ALL ANY
> ICMPTYPE
> %token ICMP6TYPE CODE KEEP MODULATE STATE PORT BINATTO NODF
> -%token MINTTL ERROR ALLOWOPTS FILENAME ROUTETO DUPTO REPLYTO NO LABEL
> +%token MINTTL ERROR ALLOWOPTS FILENAME ROUTETO DUPTO REPLYTO YES NO
> LABEL
> %token NOROUTE URPFFAILED FRAGMENT USER GROUP MAXMSS MAXIMUM TTL TOS
> DROP TABLE
> %token REASSEMBLE ANCHOR SYNCOOKIES
> %token SET OPTIMIZATION TIMEOUT LIMIT LOGINTERFACE BLOCKPOLICY RANDOMID
> @@ -3754,16 +3756,11 @@ comma : ','
> ;
>
> yesno : NO { $$ = 0; }
> - | STRING {
> - if (!strcmp($1, "yes"))
> - $$ = 1;
> - else {
> - yyerror("invalid value '%s', expected 'yes' "
> - "or 'no'", $1);
> - free($1);
> - YYERROR;
> - }
> - free($1);
> + | YES { $$ = 1; }
> + | error {
> + yyerror("invalid value '%s', expected 'yes' or 'no'",
> + yytext);
> + YYABORT;
> }
> ;
>
> @@ -5048,6 +5045,7 @@ lookup(char *s)
> { "urpf-failed", URPFFAILED},
> { "user", USER},
> { "weight", WEIGHT},
> + { "yes", YES},
> };
> const struct keywords *p;
>
> @@ -5170,10 +5168,12 @@ findeol(void)
> int
> yylex(void)
> {
> - u_char buf[8096];
> + static u_char buf[8192];
> u_char *p, *val;
> int quotec, next, c;
> int token;
> +
> + yytext = buf;
>
> top:
> p = buf;
>
>
>
