On Tue, Apr 21, 2020 at 03:37:04PM +0200, Ruediger Pluem wrote:
> Looks good in general apart from the comment below.
>
> On 4/21/20 2:53 PM, Joe Orton wrote:
> > @@ -1058,7 +1104,24 @@ AP_DECLARE_NONSTD(const char *)
> > ap_set_listener(cmd_parms *cmd, void *dummy,
> > return "Port must be specified";
> > }
> >
> > - if (argc != 2) {
> > + if (argc == 3) {
> > + if (strncasecmp(argv[2], "options=", 8)) {
> > + return "Third argument to Listen must be options=...";
> > + }
> > +
> > + err = parse_listen_flags(cmd->temp_pool, argv[2] + 8, &flags);
> > + if (err) {
> > + return err;
> > + }
> > + }
> > +
> > + if (argc == 2 && strncasecmp(argv[1], "options=", 8) == 0) {
> > + err = parse_listen_flags(cmd->temp_pool, argv[1] + 8, &flags);
> > + if (err) {
> > + return err;
> > + }
> > + }
> > + else if (argc < 3) {
>
> Shouldn't that be argc < 2?
It is < 3 to make the second arg truly optional, so a proto default is
picked either for the 1-arg form or for the 2-arg form where the second
arg is options= (i.e. first if condition applies) i.e.
Listen 80 options=foo
Making the protocol option truly optional was one of the review feedback
comments in the original thread.
No strong opinion here on whether that's right, but it reminds me that
it leads to a fragile outcome, e.g.
Listen 80 optons=foo
silently succeeds with "optons=foo" as the protocol name (not sure what
it does with it). If it is safe to assume "=" can never appear in a
protocol name, we could catch any proto with "=" and make that a config
error again.
Alternatively, can simply make the protocol a required option if
options= is used.
Regards, Joe
