Re: [RFC] optional Listen options= argument
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
Re: [RFC] optional Listen options= argument
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.
commit e9161f977d7633baf1ef34890f0de31058353c97
Author: Joe Orton
Date: Mon Apr 20 16:22:06 2020 +0100
Add "ListenFree" directive to create listener with APR_SO_FREEBIND set
(for use with an IP address which is not yet available on the system).
Reimplement "use_specific_errors" listener flag under generic
ap_listen_rec flags field to hold both these options.
* include/ap_listen.h: Add AP_LISTEN_* flags.
(ap_listen_rec): Rename use_specific_errors to flags.
* server/listen.c (make_sock): Set APR_SO_FREEBIND if
AP_LISTEN_FREEBIND flag is set on listener.
(alloc_listener): Take flags argument.
(ap_setup_listeners): Set AP_LISTEN_SPECIFIC_ERRORS flag here.
(ap_set_listener): Parse optional options=... argument.
(ap_duplicate_listeners): Duplicate flags.
Submitted by: jkaluza, Lubos Uhliarik , jorton
PR: 61865
diff --git a/include/ap_listen.h b/include/ap_listen.h
index 9cbaaa4910..2329cae70c 100644
--- a/include/ap_listen.h
+++ b/include/ap_listen.h
@@ -38,6 +38,11 @@ typedef struct ap_slave_t ap_slave_t;
typedef struct ap_listen_rec ap_listen_rec;
typedef apr_status_t (*accept_function)(void **csd, ap_listen_rec *lr,
apr_pool_t *ptrans);
+/* Flags for ap_listen_rec.flags */
+#define AP_LISTEN_SPECIFIC_ERRORS (0x0001)
+#define AP_LISTEN_FREEBIND(0x0002)
+#define AP_LISTEN_REUSEPORT (0x0004)
+
/**
* @brief Apache's listeners record.
*
@@ -73,10 +78,9 @@ struct ap_listen_rec {
ap_slave_t *slave;
/**
- * Allow the accept_func to return a wider set of return codes
+ * Various AP_LISTEN_* flags.
*/
-int use_specific_errors;
-
+apr_uint32_t flags;
};
/**
diff --git a/include/ap_mmn.h b/include/ap_mmn.h
index 24ac648ac9..1271ce18ed 100644
--- a/include/ap_mmn.h
+++ b/include/ap_mmn.h
@@ -628,14 +628,15 @@
* 20200331.2 (2.5.1-dev) Add ap_proxy_should_override to mod_proxy.h
* 20200331.3 (2.5.1-dev) Add ap_parse_request_line() and
* ap_check_request_header()
+ * 20200420.0 (2.5.1-dev) Add flags to listen_rec in place of
use_specific_errors
*/
#define MODULE_MAGIC_COOKIE 0x41503235UL /* "AP25" */
#ifndef MODULE_MAGIC_NUMBER_MAJOR
-#define MODULE_MAGIC_NUMBER_MAJOR 20200331
+#define MODULE_MAGIC_NUMBER_MAJOR 20200420
#endif
-#define MODULE_MAGIC_NUMBER_MINOR 3/* 0...n */
+#define MODULE_MAGIC_NUMBER_MINOR 0/* 0...n */
/**
* Determine if the server's current MODULE_MAGIC_NUMBER is at least a
diff --git a/os/unix/unixd.c b/os/unix/unixd.c
index bde859022b..3b0e695727 100644
--- a/os/unix/unixd.c
+++ b/os/unix/unixd.c
@@ -323,7 +323,7 @@ AP_DECLARE(apr_status_t) ap_unixd_accept(void **accepted,
ap_listen_rec *lr,
}
/* Let the caller handle slightly more varied return values */
-if (lr->use_specific_errors && ap_accept_error_is_nonfatal(status)) {
+if ((lr->flags & AP_LISTEN_SPECIFIC_ERRORS) &&
ap_accept_error_is_nonfatal(status)) {
return status;
}
diff --git a/server/listen.c b/server/listen.c
index 87a4bafe80..f9c8ff5a54 100644
--- a/server/listen.c
+++ b/server/listen.c
@@ -150,7 +150,8 @@ static apr_status_t make_sock(apr_pool_t *p, ap_listen_rec
*server, int do_bind_
#endif
#if defined(SO_REUSEPORT)
-if (ap_have_so_reuseport && ap_listencbratio > 0) {
+if (server->flags & AP_LISTEN_REUSEPORT
+|| (ap_have_so_reuseport && ap_listencbratio > 0)) {
int thesock;
apr_os_sock_get(, s);
if (setsockopt(thesock, SOL_SOCKET, SO_REUSEPORT,
@@ -166,6 +167,21 @@ static apr_status_t make_sock(apr_pool_t *p, ap_listen_rec
*server, int do_bind_
}
#endif
+
+#if defined(APR_SO_FREEBIND)
+if (server->flags & AP_LISTEN_FREEBIND) {
+if (apr_socket_opt_set(s, APR_SO_FREEBIND, one) < 0) {
+stat = apr_get_netos_error();
+ap_log_perror(APLOG_MARK, APLOG_CRIT, stat, p, APLOGNO()
+ "make_sock: apr_socket_opt_set: "
+ "error setting APR_SO_FREEBIND");
+apr_socket_close(s);
+return stat;
+}
+}
+#endif
+
+
if (do_bind_listen) {
#if APR_HAVE_IPV6
if (server->bind_addr->family == APR_INET6) {
@@ -467,7 +483,7 @@ static int find_listeners(ap_listen_rec **from,
ap_listen_rec **to,
static const char *alloc_listener(process_rec *process, const char *addr,
apr_port_t port, const char* proto,
const char *scope_id, void *slave,
-
Re: [RFC] optional Listen options= argument
On Tue, Apr 21, 2020 at 4:13 PM Joe Orton 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
On Tue, Apr 21, 2020 at 2:53 PM Joe Orton 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
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, );
> > +if (err) {
> > +return err;
> > +}
> > +}
> > +
> > +if (argc == 2 && strncasecmp(argv[1], "options=", 8) == 0) {
> > +err = parse_listen_flags(cmd->temp_pool, argv[1] + 8, );
> > +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
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, );
> +if (err) {
> +return err;
> +}
> +}
> +
> +if (argc == 2 && strncasecmp(argv[1], "options=", 8) == 0) {
> +err = parse_listen_flags(cmd->temp_pool, argv[1] + 8, );
> +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
On Tue, Apr 21, 2020 at 3:12 PM Eric Covener wrote: > > On Tue, Apr 21, 2020 at 8:53 AM Joe Orton 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.2040...@redhat.com%3e > > > LGTM +1 very nice
Re: [RFC] optional Listen options= argument
On Tue, Apr 21, 2020 at 8:53 AM Joe Orton 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.2040...@redhat.com%3e LGTM -- Eric Covener cove...@gmail.com
[RFC] optional Listen options= argument
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.2040...@redhat.com%3e
diff --git a/include/ap_listen.h b/include/ap_listen.h
index 9cbaaa4910..2329cae70c 100644
--- a/include/ap_listen.h
+++ b/include/ap_listen.h
@@ -38,6 +38,11 @@ typedef struct ap_slave_t ap_slave_t;
typedef struct ap_listen_rec ap_listen_rec;
typedef apr_status_t (*accept_function)(void **csd, ap_listen_rec *lr,
apr_pool_t *ptrans);
+/* Flags for ap_listen_rec.flags */
+#define AP_LISTEN_SPECIFIC_ERRORS (0x0001)
+#define AP_LISTEN_FREEBIND(0x0002)
+#define AP_LISTEN_REUSEPORT (0x0004)
+
/**
* @brief Apache's listeners record.
*
@@ -73,10 +78,9 @@ struct ap_listen_rec {
ap_slave_t *slave;
/**
- * Allow the accept_func to return a wider set of return codes
+ * Various AP_LISTEN_* flags.
*/
-int use_specific_errors;
-
+apr_uint32_t flags;
};
/**
diff --git a/include/ap_mmn.h b/include/ap_mmn.h
index 24ac648ac9..1271ce18ed 100644
--- a/include/ap_mmn.h
+++ b/include/ap_mmn.h
@@ -628,14 +628,15 @@
* 20200331.2 (2.5.1-dev) Add ap_proxy_should_override to mod_proxy.h
* 20200331.3 (2.5.1-dev) Add ap_parse_request_line() and
* ap_check_request_header()
+ * 20200420.0 (2.5.1-dev) Add flags to listen_rec in place of
use_specific_errors
*/
#define MODULE_MAGIC_COOKIE 0x41503235UL /* "AP25" */
#ifndef MODULE_MAGIC_NUMBER_MAJOR
-#define MODULE_MAGIC_NUMBER_MAJOR 20200331
+#define MODULE_MAGIC_NUMBER_MAJOR 20200420
#endif
-#define MODULE_MAGIC_NUMBER_MINOR 3/* 0...n */
+#define MODULE_MAGIC_NUMBER_MINOR 0/* 0...n */
/**
* Determine if the server's current MODULE_MAGIC_NUMBER is at least a
diff --git a/os/unix/unixd.c b/os/unix/unixd.c
index bde859022b..3b0e695727 100644
--- a/os/unix/unixd.c
+++ b/os/unix/unixd.c
@@ -323,7 +323,7 @@ AP_DECLARE(apr_status_t) ap_unixd_accept(void **accepted,
ap_listen_rec *lr,
}
/* Let the caller handle slightly more varied return values */
-if (lr->use_specific_errors && ap_accept_error_is_nonfatal(status)) {
+if ((lr->flags & AP_LISTEN_SPECIFIC_ERRORS) &&
ap_accept_error_is_nonfatal(status)) {
return status;
}
diff --git a/server/listen.c b/server/listen.c
index 87a4bafe80..3657ba65c7 100644
--- a/server/listen.c
+++ b/server/listen.c
@@ -150,7 +150,8 @@ static apr_status_t make_sock(apr_pool_t *p, ap_listen_rec
*server, int do_bind_
#endif
#if defined(SO_REUSEPORT)
-if (ap_have_so_reuseport && ap_listencbratio > 0) {
+if (server->flags & AP_LISTEN_REUSEPORT
+|| (ap_have_so_reuseport && ap_listencbratio > 0)) {
int thesock;
apr_os_sock_get(, s);
if (setsockopt(thesock, SOL_SOCKET, SO_REUSEPORT,
@@ -166,6 +167,21 @@ static apr_status_t make_sock(apr_pool_t *p, ap_listen_rec
*server, int do_bind_
}
#endif
+
+#if defined(APR_SO_FREEBIND)
+if (server->flags & AP_LISTEN_FREEBIND) {
+if (apr_socket_opt_set(s, APR_SO_FREEBIND, one) < 0) {
+stat = apr_get_netos_error();
+ap_log_perror(APLOG_MARK, APLOG_CRIT, stat, p, APLOGNO()
+ "make_sock: apr_socket_opt_set: "
+ "error setting APR_SO_FREEBIND");
+apr_socket_close(s);
+return stat;
+ }
+ }
+#endif
+
+
if (do_bind_listen) {
#if APR_HAVE_IPV6
if (server->bind_addr->family == APR_INET6) {
@@ -467,7 +483,7 @@ static int find_listeners(ap_listen_rec **from,
ap_listen_rec **to,
static const char *alloc_listener(process_rec *process, const char *addr,
apr_port_t port, const char* proto,
const char *scope_id, void *slave,
- apr_pool_t *temp_pool)
+ apr_pool_t *temp_pool, apr_uint32_t flags)
{
ap_listen_rec *last;
apr_status_t status;
@@ -511,6 +527,7 @@ static const char *alloc_listener(process_rec *process,
const char *addr,
new->next = 0;
new->bind_addr = sa;
