Mailing List Archive: [RFC] optional Listen options= argument

[RFC] optional Listen options= argument

jorton at redhat

Apr 21, 2020, 5:53 AM

Post #1 of 9 (408 views)

A previous conversation [1] about optionally enabling socket options for
Listen seemed to gain consensus around adding an optional argument,
rather than multiple alternative Listen-like directives.

I've attached a patch based on work by both Jan Kaluza and Lubos
Uhliarik which implements this, updated for trunk. Syntax used is:

Listen [IP-address:]portnumber [protocol] [options=keyword,keyword,...]

where options is a comma-separated list of keywords. In this patch the
"reuseport" and "freebind" keywords are supported for APR_SO_REUSEPORT
and APR_SO_FREEBIND respectively. Key/value keywords could be used to
allow per-listener backlog, TCP buffer sizes etc, though non-boolean
options will require extending ap_listen_rec so that needs care.

Opinions? Is there still consensus that extending Listen like this is a
good idea?

Regards, Joe

[1] http://mail-archives.apache.org/mod_mbox/httpd-dev/201603.mbox/%3c56DD68E5.2040309@redhat.com%3e

Re: [RFC] optional Listen options= argument [ In reply to ]

covener at gmail

Apr 21, 2020, 6:01 AM

Post #2 of 9 (408 views)

On Tue, Apr 21, 2020 at 8:53 AM Joe Orton <jorton@redhat.com> wrote:
>
> A previous conversation [1] about optionally enabling socket options for
> Listen seemed to gain consensus around adding an optional argument,
> rather than multiple alternative Listen-like directives.
>
> I've attached a patch based on work by both Jan Kaluza and Lubos
> Uhliarik which implements this, updated for trunk. Syntax used is:
>
> Listen [IP-address:]portnumber [protocol] [options=keyword,keyword,...]
>
> where options is a comma-separated list of keywords. In this patch the
> "reuseport" and "freebind" keywords are supported for APR_SO_REUSEPORT
> and APR_SO_FREEBIND respectively. Key/value keywords could be used to
> allow per-listener backlog, TCP buffer sizes etc, though non-boolean
> options will require extending ap_listen_rec so that needs care.
>
> Opinions? Is there still consensus that extending Listen like this is a
> good idea?
>
> Regards, Joe
>
> [1] http://mail-archives.apache.org/mod_mbox/httpd-dev/201603.mbox/%3c56DD68E5.2040309@redhat.com%3e

LGTM

--
Eric Covener
covener@gmail.com

Re: [RFC] optional Listen options= argument [ In reply to ]

toscano.luca at gmail

Apr 21, 2020, 6:26 AM

Post #3 of 9 (408 views)

On Tue, Apr 21, 2020 at 3:12 PM Eric Covener <covener@gmail.com> wrote:
>
> On Tue, Apr 21, 2020 at 8:53 AM Joe Orton <jorton@redhat.com> wrote:
> >
> > A previous conversation [1] about optionally enabling socket options for
> > Listen seemed to gain consensus around adding an optional argument,
> > rather than multiple alternative Listen-like directives.
> >
> > I've attached a patch based on work by both Jan Kaluza and Lubos
> > Uhliarik which implements this, updated for trunk. Syntax used is:
> >
> > Listen [IP-address:]portnumber [protocol] [options=keyword,keyword,...]
> >
> > where options is a comma-separated list of keywords. In this patch the
> > "reuseport" and "freebind" keywords are supported for APR_SO_REUSEPORT
> > and APR_SO_FREEBIND respectively. Key/value keywords could be used to
> > allow per-listener backlog, TCP buffer sizes etc, though non-boolean
> > options will require extending ap_listen_rec so that needs care.
> >
> > Opinions? Is there still consensus that extending Listen like this is a
> > good idea?
> >
> > Regards, Joe
> >
> > [1] http://mail-archives.apache.org/mod_mbox/httpd-dev/201603.mbox/%3c56DD68E5.2040309@redhat.com%3e
>
>
> LGTM

+1 very nice

Re: [RFC] optional Listen options= argument [ In reply to ]

rpluem at apache

Apr 21, 2020, 6:37 AM

Post #4 of 9 (408 views)

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?

> if (port == 443) {
> proto = "https";
> } else {

Regards

Rüdiger

Re: [RFC] optional Listen options= argument [ In reply to ]

jorton at redhat

Apr 21, 2020, 7:13 AM

Post #5 of 9 (408 views)

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

Re: [RFC] optional Listen options= argument [ In reply to ]

ylavic.dev at gmail

Apr 21, 2020, 7:33 AM

Post #6 of 9 (408 views)

On Tue, Apr 21, 2020 at 2:53 PM Joe Orton <jorton@redhat.com> wrote:
>
> Opinions? Is there still consensus that extending Listen like this is a
> good idea?

+1, nice.

I also like that we can set reuseport without ListenCoresBucketsRatio
> 0, both are orthogonal I think.

Re: [RFC] optional Listen options= argument [ In reply to ]

ylavic.dev at gmail

Apr 21, 2020, 7:52 AM

Post #7 of 9 (408 views)

On Tue, Apr 21, 2020 at 4:13 PM Joe Orton <jorton@redhat.com> wrote:
>
> 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.

Looks sensible to me, the proto is supposed to be a scheme (so ALNUM
or [.+-], IIRC).

Re: [RFC] optional Listen options= argument [ In reply to ]

jorton at redhat

Apr 21, 2020, 9:02 AM

Post #8 of 9 (408 views)

On Tue, Apr 21, 2020 at 03:13:47PM +0100, Joe Orton wrote:
> On Tue, Apr 21, 2020 at 03:37:04PM +0200, Ruediger Pluem wrote:
> > Shouldn't that be argc < 2?
>
> It is < 3 to make the second arg truly optional, so a proto default is

No, you were right the logic was borked.

Updated patch which fixes this, and checks the protocol name for the
obvious typo problem attached. Thanks for reviews.

Re: [RFC] optional Listen options= argument [ In reply to ]

rpluem at apache

Apr 21, 2020, 11:22 PM

Post #9 of 9 (399 views)

On 4/21/20 6:02 PM, Joe Orton wrote:
> On Tue, Apr 21, 2020 at 03:13:47PM +0100, Joe Orton wrote:
>> On Tue, Apr 21, 2020 at 03:37:04PM +0200, Ruediger Pluem wrote:
>>> Shouldn't that be argc < 2?
>>
>> It is < 3 to make the second arg truly optional, so a proto default is
>
> No, you were right the logic was borked.
>
> Updated patch which fixes this, and checks the protocol name for the
> obvious typo problem attached. Thanks for reviews.
>

Welcome. Looks good now.

Regards

Rüdiger