On Thu, Feb 06, 2020 at 03:33:45PM +0100, Tim Düsterhus wrote: > Willy, > > Am 06.02.20 um 15:24 schrieb Willy Tarreau: > > I was surprized because I discovered that we've always accepted empty > > expressions thus"if" without even a condition or "if or" are valid, which > > adds to the confusion. So your patch does indeed make sense. However we > > don't need to test for "||" because it's already invalid as an ACL name > > and the existing message is more precise about it (invalid chars in the > > name). > > I added the test for '||' for completeness in case there are adjustments > to valid characters later on. I am aware that it will never trigger and > being only run on configuration parsing the performance impact should be > a non-issue.
It's not a matter of performance impact but that the message is misleading in this case. It says that "||" is forbidden as an ACL name, which sort of implies a lot of other ones are valid. It's not "||" that's forbidden, it's anything not made exclusively of "[0-9a-zA-Z_.:-]", and actually your message scared me and made me recheck the doc to be sure. > > Also I'd rather have the "goto out" statement after setting the error, > > otherwise it becomes confusing and will certainly trap someone adding > > yet another test later. > > So you want to make using 'or' as an ACL name fatal instead of a warning? Ah silly me! I misread it, I read only the first one as a warning and the second one as an error. However you're making me think now :-) We should likely make it an error in 2.2 and a warning in existing branches. We know that people don't read warnings, so if we keep it forever, such a config might last. But we must not break an existing config in a stable branch otherwise users roll back by lack of time to fix the conf. > Feel free to either modify the patch yourself to make the adjustments > you want to see or let me know if you want me to make them. It might > require a bit more back and forth, though :-) OK I guess I can do them, sure. Thanks! Willy
