* patch: debuginfod ipv4-ipv6 dual-stack
@ 2022-04-03 23:49 Frank Ch. Eigler
2022-04-04 16:58 ` Mark Wielaard
0 siblings, 1 reply; 3+ messages in thread
From: Frank Ch. Eigler @ 2022-04-03 23:49 UTC (permalink / raw)
To: elfutils-devel
Hi -
This little improvement reduces wasted memory from duplicated
worker threads for ipv4 vs ipv6 stacks.
commit 3a00f412b6554ba70f7b7e3d3aa6f67f278868af (HEAD -> master)
Author: Frank Ch. Eigler <fche@redhat.com>
Date: Sun Apr 3 19:42:48 2022 -0400
debuginfod: use single ipv4+ipv6 microhttpd daemon configuration
Use a single MHD_USE_DUAL_STACK mhd daemon. This way, the thread
connection pool is not doubled, saving memory and better matching user
expectations. A slight tweak to logging is required to pull IPv4
remote addresses back out, and also to allow IPv6 ::-laden address
forwarding through federation links.
Signed-off-by: Frank Ch. Eigler <fche@redhat.com>
diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 38a389e7dfd3..578d951d0102 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,12 @@
+2022-04-03 Frank Ch. Eigler <fche@redhat.com>
+
+ * debuginfod.cxx (main): Use single dual-stack daemon setup,
+ rather than duplicate ipv4 and ipv6.
+ (conninfo, handle_buildid): Represent ipv4-mapped ipv6 addresses
+ in their native ipv4 form for logging and X-F-F: purposes.
+ * debuginfod-client.c (debuginfod_add_http_header): Tolerate
+ colons in http header values.
+
2022-04-03 Frank Ch. Eigler <fche@redhat.com>
* debuginfod.cxx (main): Use MHD_USE_EPOLL for libmicrohttpd, to
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 024b09545d99..41ba88a56911 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -1548,13 +1548,13 @@ int debuginfod_find_source(debuginfod_client *client,
int debuginfod_add_http_header (debuginfod_client *client, const char* header)
{
/* Sanity check header value is of the form Header: Value.
- It should contain exactly one colon that isn't the first or
+ It should contain at least one colon that isn't the first or
last character. */
- char *colon = strchr (header, ':');
- if (colon == NULL
- || colon == header
- || *(colon + 1) == '\0'
- || strchr (colon + 1, ':') != NULL)
+ char *colon = strchr (header, ':'); /* first colon */
+ if (colon == NULL /* present */
+ || colon == header /* not at beginning - i.e., have a header name */
+ || *(colon + 1) == '\0') /* not at end - i.e., have a value */
+ /* NB: but it's okay for a value to contain other colons! */
return -EINVAL;
struct curl_slist *temp = curl_slist_append (client->headers, header);
diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
index 99924d36f260..48e0f37b33f0 100644
--- a/debuginfod/debuginfod.cxx
+++ b/debuginfod/debuginfod.cxx
@@ -1063,9 +1063,22 @@ conninfo (struct MHD_Connection * conn)
sts = getnameinfo (so, sizeof (struct sockaddr_in), hostname, sizeof (hostname), servname,
sizeof (servname), NI_NUMERICHOST | NI_NUMERICSERV);
} else if (so && so->sa_family == AF_INET6) {
- sts = getnameinfo (so, sizeof (struct sockaddr_in6), hostname, sizeof (hostname),
- servname, sizeof (servname), NI_NUMERICHOST | NI_NUMERICSERV);
+ struct sockaddr_in6* addr6 = (struct sockaddr_in6*) so;
+ if (IN6_IS_ADDR_V4MAPPED(&addr6->sin6_addr)) {
+ struct sockaddr_in addr4;
+ memset (&addr4, 0, sizeof(addr4));
+ addr4.sin_family = AF_INET;
+ addr4.sin_port = addr6->sin6_port;
+ memcpy (&addr4.sin_addr.s_addr, addr6->sin6_addr.s6_addr+12, sizeof(addr4.sin_addr.s_addr));
+ sts = getnameinfo ((struct sockaddr*) &addr4, sizeof (addr4),
+ hostname, sizeof (hostname), servname, sizeof (servname),
+ NI_NUMERICHOST | NI_NUMERICSERV);
+ } else {
+ sts = getnameinfo (so, sizeof (struct sockaddr_in6), hostname, sizeof (hostname), NULL, 0,
+ NI_NUMERICHOST);
+ }
}
+
if (sts != 0) {
hostname[0] = servname[0] = '\0';
}
@@ -2008,13 +2021,26 @@ and will not query the upstream servers");
MHD_CONNECTION_INFO_CLIENT_ADDRESS);
struct sockaddr *so = u ? u->client_addr : 0;
char hostname[256] = ""; // RFC1035
- if (so && so->sa_family == AF_INET)
+ if (so && so->sa_family == AF_INET) {
(void) getnameinfo (so, sizeof (struct sockaddr_in), hostname, sizeof (hostname), NULL, 0,
NI_NUMERICHOST);
- else if (so && so->sa_family == AF_INET6)
- (void) getnameinfo (so, sizeof (struct sockaddr_in6), hostname, sizeof (hostname), NULL, 0,
- NI_NUMERICHOST);
-
+ } else if (so && so->sa_family == AF_INET6) {
+ struct sockaddr_in6* addr6 = (struct sockaddr_in6*) so;
+ if (IN6_IS_ADDR_V4MAPPED(&addr6->sin6_addr)) {
+ struct sockaddr_in addr4;
+ memset (&addr4, 0, sizeof(addr4));
+ addr4.sin_family = AF_INET;
+ addr4.sin_port = addr6->sin6_port;
+ memcpy (&addr4.sin_addr.s_addr, addr6->sin6_addr.s6_addr+12, sizeof(addr4.sin_addr.s_addr));
+ (void) getnameinfo ((struct sockaddr*) &addr4, sizeof (addr4),
+ hostname, sizeof (hostname), NULL, 0,
+ NI_NUMERICHOST);
+ } else {
+ (void) getnameinfo (so, sizeof (struct sockaddr_in6), hostname, sizeof (hostname), NULL, 0,
+ NI_NUMERICHOST);
+ }
+ }
+
string xff_complete = string("X-Forwarded-For: ")+xff+string(hostname);
debuginfod_add_http_header (client, xff_complete.c_str());
}
@@ -3873,26 +3899,8 @@ main (int argc, char *argv[])
}
}
- // Start httpd server threads. Separate pool for IPv4 and IPv6, in
- // case the host only has one protocol stack.
- MHD_Daemon *d4 = MHD_start_daemon ((connection_pool ? 0 : MHD_USE_THREAD_PER_CONNECTION)
-#if MHD_VERSION >= 0x00095300
- | MHD_USE_INTERNAL_POLLING_THREAD
-#else
- | MHD_USE_SELECT_INTERNALLY
-#endif
-#ifdef MHD_USE_EPOLL
- | MHD_USE_EPOLL
-#endif
- | MHD_USE_DEBUG, /* report errors to stderr */
- http_port,
- NULL, NULL, /* default accept policy */
- handler_cb, NULL, /* handler callback */
- MHD_OPTION_EXTERNAL_LOGGER, error_cb, NULL,
- (connection_pool ? MHD_OPTION_THREAD_POOL_SIZE : MHD_OPTION_END),
- (connection_pool ? (int)connection_pool : MHD_OPTION_END),
- MHD_OPTION_END);
- MHD_Daemon *d6 = MHD_start_daemon ((connection_pool ? 0 : MHD_USE_THREAD_PER_CONNECTION)
+ // Start httpd server threads. Use a single dual-homed pool.
+ MHD_Daemon *d46 = MHD_start_daemon ((connection_pool ? 0 : MHD_USE_THREAD_PER_CONNECTION)
#if MHD_VERSION >= 0x00095300
| MHD_USE_INTERNAL_POLLING_THREAD
#else
@@ -3901,7 +3909,7 @@ main (int argc, char *argv[])
#ifdef MHD_USE_EPOLL
| MHD_USE_EPOLL
#endif
- | MHD_USE_IPv6
+ | MHD_USE_DUAL_STACK
| MHD_USE_DEBUG, /* report errors to stderr */
http_port,
NULL, NULL, /* default accept policy */
@@ -3911,7 +3919,7 @@ main (int argc, char *argv[])
(connection_pool ? (int)connection_pool : MHD_OPTION_END),
MHD_OPTION_END);
- if (d4 == NULL && d6 == NULL) // neither ipv4 nor ipv6? boo
+ if (d46 == NULL)
{
sqlite3 *database = db;
sqlite3 *databaseq = dbq;
@@ -3922,8 +3930,8 @@ main (int argc, char *argv[])
}
obatched(clog) << "started http server on "
- << (d4 != NULL ? "IPv4 " : "")
- << (d6 != NULL ? "IPv6 " : "")
+ << "IPv4 "
+ << "IPv6 "
<< "port=" << http_port << endl;
// add maxigroom sql if -G given
@@ -4043,8 +4051,7 @@ main (int argc, char *argv[])
pthread_join (it, NULL);
/* Stop all the web service threads. */
- if (d4) MHD_stop_daemon (d4);
- if (d6) MHD_stop_daemon (d6);
+ if (d46) MHD_stop_daemon (d46);
if (! passive_p)
{
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: patch: debuginfod ipv4-ipv6 dual-stack
2022-04-03 23:49 patch: debuginfod ipv4-ipv6 dual-stack Frank Ch. Eigler
@ 2022-04-04 16:58 ` Mark Wielaard
2022-04-04 17:03 ` Frank Ch. Eigler
0 siblings, 1 reply; 3+ messages in thread
From: Mark Wielaard @ 2022-04-04 16:58 UTC (permalink / raw)
To: Frank Ch. Eigler, elfutils-devel
Hi Frank,
On Sun, 2022-04-03 at 19:49 -0400, Frank Ch. Eigler via Elfutils-devel wrote:
> commit 3a00f412b6554ba70f7b7e3d3aa6f67f278868af (HEAD -> master)
> Author: Frank Ch. Eigler <fche@redhat.com>
> Date: Sun Apr 3 19:42:48 2022 -0400
>
> debuginfod: use single ipv4+ipv6 microhttpd daemon configuration
>
> Use a single MHD_USE_DUAL_STACK mhd daemon. This way, the thread
> connection pool is not doubled, saving memory and better matching user
> expectations. A slight tweak to logging is required to pull IPv4
> remote addresses back out, and also to allow IPv6 ::-laden address
> forwarding through federation links.
>
> Signed-off-by: Frank Ch. Eigler <fche@redhat.com>
>
> diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
> index 38a389e7dfd3..578d951d0102 100644
> --- a/debuginfod/ChangeLog
> +++ b/debuginfod/ChangeLog
> @@ -1,3 +1,12 @@
> +2022-04-03 Frank Ch. Eigler <fche@redhat.com>
> +
> + * debuginfod.cxx (main): Use single dual-stack daemon setup,
> + rather than duplicate ipv4 and ipv6.
> + (conninfo, handle_buildid): Represent ipv4-mapped ipv6 addresses
> + in their native ipv4 form for logging and X-F-F: purposes.
> + * debuginfod-client.c (debuginfod_add_http_header): Tolerate
> + colons in http header values.
This looks ok. Some comments below, but nothing I think needs to
change.
> 2022-04-03 Frank Ch. Eigler <fche@redhat.com>
>
> * debuginfod.cxx (main): Use MHD_USE_EPOLL for libmicrohttpd, to
> diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
> index 024b09545d99..41ba88a56911 100644
> --- a/debuginfod/debuginfod-client.c
> +++ b/debuginfod/debuginfod-client.c
> @@ -1548,13 +1548,13 @@ int debuginfod_find_source(debuginfod_client *client,
> int debuginfod_add_http_header (debuginfod_client *client, const char* header)
> {
> /* Sanity check header value is of the form Header: Value.
> - It should contain exactly one colon that isn't the first or
> + It should contain at least one colon that isn't the first or
> last character. */
> - char *colon = strchr (header, ':');
> - if (colon == NULL
> - || colon == header
> - || *(colon + 1) == '\0'
> - || strchr (colon + 1, ':') != NULL)
> + char *colon = strchr (header, ':'); /* first colon */
> + if (colon == NULL /* present */
> + || colon == header /* not at beginning - i.e., have a header name */
> + || *(colon + 1) == '\0') /* not at end - i.e., have a value */
> + /* NB: but it's okay for a value to contain other colons! */
> return -EINVAL;
OK.
> struct curl_slist *temp = curl_slist_append (client->headers, header);
> diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx
> index 99924d36f260..48e0f37b33f0 100644
> --- a/debuginfod/debuginfod.cxx
> +++ b/debuginfod/debuginfod.cxx
> @@ -1063,9 +1063,22 @@ conninfo (struct MHD_Connection * conn)
> sts = getnameinfo (so, sizeof (struct sockaddr_in), hostname, sizeof (hostname), servname,
> sizeof (servname), NI_NUMERICHOST | NI_NUMERICSERV);
> } else if (so && so->sa_family == AF_INET6) {
> - sts = getnameinfo (so, sizeof (struct sockaddr_in6), hostname, sizeof (hostname),
> - servname, sizeof (servname), NI_NUMERICHOST | NI_NUMERICSERV);
> + struct sockaddr_in6* addr6 = (struct sockaddr_in6*) so;
> + if (IN6_IS_ADDR_V4MAPPED(&addr6->sin6_addr)) {
> + struct sockaddr_in addr4;
> + memset (&addr4, 0, sizeof(addr4));
> + addr4.sin_family = AF_INET;
> + addr4.sin_port = addr6->sin6_port;
> + memcpy (&addr4.sin_addr.s_addr, addr6->sin6_addr.s6_addr+12, sizeof(addr4.sin_addr.s_addr));
> + sts = getnameinfo ((struct sockaddr*) &addr4, sizeof (addr4),
> + hostname, sizeof (hostname), servname, sizeof (servname),
> + NI_NUMERICHOST | NI_NUMERICSERV);
> + } else {
> + sts = getnameinfo (so, sizeof (struct sockaddr_in6), hostname, sizeof (hostname), NULL, 0,
> + NI_NUMERICHOST);
> + }
> }
That IN6_IS_ADDR_V4MAPPED is funny. Is that actually used in practice?
> if (sts != 0) {
> hostname[0] = servname[0] = '\0';
> }
> @@ -2008,13 +2021,26 @@ and will not query the upstream servers");
> MHD_CONNECTION_INFO_CLIENT_ADDRESS);
> struct sockaddr *so = u ? u->client_addr : 0;
> char hostname[256] = ""; // RFC1035
> - if (so && so->sa_family == AF_INET)
> + if (so && so->sa_family == AF_INET) {
> (void) getnameinfo (so, sizeof (struct sockaddr_in), hostname, sizeof (hostname), NULL, 0,
> NI_NUMERICHOST);
> - else if (so && so->sa_family == AF_INET6)
> - (void) getnameinfo (so, sizeof (struct sockaddr_in6), hostname, sizeof (hostname), NULL, 0,
> - NI_NUMERICHOST);
> -
> + } else if (so && so->sa_family == AF_INET6) {
> + struct sockaddr_in6* addr6 = (struct sockaddr_in6*) so;
> + if (IN6_IS_ADDR_V4MAPPED(&addr6->sin6_addr)) {
> + struct sockaddr_in addr4;
> + memset (&addr4, 0, sizeof(addr4));
> + addr4.sin_family = AF_INET;
> + addr4.sin_port = addr6->sin6_port;
> + memcpy (&addr4.sin_addr.s_addr, addr6->sin6_addr.s6_addr+12, sizeof(addr4.sin_addr.s_addr));
> + (void) getnameinfo ((struct sockaddr*) &addr4, sizeof (addr4),
> + hostname, sizeof (hostname), NULL, 0,
> + NI_NUMERICHOST);
> + } else {
> + (void) getnameinfo (so, sizeof (struct sockaddr_in6), hostname, sizeof (hostname), NULL, 0,
> + NI_NUMERICHOST);
> + }
> + }
Too bad this cannot be merged with conninfo above.
But this calculates less info.
> string xff_complete = string("X-Forwarded-For: ")+xff+string(hostname);
> debuginfod_add_http_header (client, xff_complete.c_str());
> }
> @@ -3873,26 +3899,8 @@ main (int argc, char *argv[])
> }
> }
>
> - // Start httpd server threads. Separate pool for IPv4 and IPv6, in
> - // case the host only has one protocol stack.
> - MHD_Daemon *d4 = MHD_start_daemon ((connection_pool ? 0 : MHD_USE_THREAD_PER_CONNECTION)
> -#if MHD_VERSION >= 0x00095300
> - | MHD_USE_INTERNAL_POLLING_THREAD
> -#else
> - | MHD_USE_SELECT_INTERNALLY
> -#endif
> -#ifdef MHD_USE_EPOLL
> - | MHD_USE_EPOLL
> -#endif
> - | MHD_USE_DEBUG, /* report errors to stderr */
> - http_port,
> - NULL, NULL, /* default accept policy */
> - handler_cb, NULL, /* handler callback */
> - MHD_OPTION_EXTERNAL_LOGGER, error_cb, NULL,
> - (connection_pool ? MHD_OPTION_THREAD_POOL_SIZE : MHD_OPTION_END),
> - (connection_pool ? (int)connection_pool : MHD_OPTION_END),
> - MHD_OPTION_END);
> - MHD_Daemon *d6 = MHD_start_daemon ((connection_pool ? 0 : MHD_USE_THREAD_PER_CONNECTION)
> + // Start httpd server threads. Use a single dual-homed pool.
> + MHD_Daemon *d46 = MHD_start_daemon ((connection_pool ? 0 : MHD_USE_THREAD_PER_CONNECTION)
> #if MHD_VERSION >= 0x00095300
> | MHD_USE_INTERNAL_POLLING_THREAD
> #else
> @@ -3901,7 +3909,7 @@ main (int argc, char *argv[])
> #ifdef MHD_USE_EPOLL
> | MHD_USE_EPOLL
> #endif
> - | MHD_USE_IPv6
> + | MHD_USE_DUAL_STACK
> | MHD_USE_DEBUG, /* report errors to stderr */
> http_port,
> NULL, NULL, /* default accept policy */
Nice cleanup.
> @@ -3911,7 +3919,7 @@ main (int argc, char *argv[])
> (connection_pool ? (int)connection_pool : MHD_OPTION_END),
> MHD_OPTION_END);
>
> - if (d4 == NULL && d6 == NULL) // neither ipv4 nor ipv6? boo
> + if (d46 == NULL)
> {
> sqlite3 *database = db;
> sqlite3 *databaseq = dbq;
> @@ -3922,8 +3930,8 @@ main (int argc, char *argv[])
> }
>
> obatched(clog) << "started http server on "
> - << (d4 != NULL ? "IPv4 " : "")
> - << (d6 != NULL ? "IPv6 " : "")
> + << "IPv4 "
> + << "IPv6 "
> << "port=" << http_port << endl;
This keeps the log output the same, but I wouldn't mind just removing
the IPv4 and IPv6 strings.
Cheers,
Mark
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: patch: debuginfod ipv4-ipv6 dual-stack
2022-04-04 16:58 ` Mark Wielaard
@ 2022-04-04 17:03 ` Frank Ch. Eigler
0 siblings, 0 replies; 3+ messages in thread
From: Frank Ch. Eigler @ 2022-04-04 17:03 UTC (permalink / raw)
To: Mark Wielaard; +Cc: elfutils-devel
Hi -
> That IN6_IS_ADDR_V4MAPPED is funny. Is that actually used in practice?
Yeah, actually all the time. On a dual-stack socket, an ipv4 connection
gets an ipv6 peer-address that renders like ::ffff:1.2.3.4.
> Too bad this cannot be merged with conninfo above.
> But this calculates less info.
Yeah.
> > obatched(clog) << "started http server on "
> > - << (d4 != NULL ? "IPv4 " : "")
> > - << (d6 != NULL ? "IPv6 " : "")
> > + << "IPv4 "
> > + << "IPv6 "
> > << "port=" << http_port << endl;
>
> This keeps the log output the same, but I wouldn't mind just removing
> the IPv4 and IPv6 strings.
Yeah, makes sense. OK, will push with that change.
- FChE
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-04-04 17:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-03 23:49 patch: debuginfod ipv4-ipv6 dual-stack Frank Ch. Eigler
2022-04-04 16:58 ` Mark Wielaard
2022-04-04 17:03 ` Frank Ch. Eigler
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).