From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (gnu.wildebeest.org [45.83.234.184]) by sourceware.org (Postfix) with ESMTPS id AF2633858418 for ; Mon, 4 Apr 2022 16:58:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org AF2633858418 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=klomp.org Received: from tarox.wildebeest.org (83-87-18-245.cable.dynamic.v4.ziggo.nl [83.87.18.245]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id C1147301FE85; Mon, 4 Apr 2022 18:58:14 +0200 (CEST) Received: by tarox.wildebeest.org (Postfix, from userid 1000) id 934AB413CD0E; Mon, 4 Apr 2022 18:58:12 +0200 (CEST) Message-ID: <998b003028dc3d65339cb6160159d96f31157570.camel@klomp.org> Subject: Re: patch: debuginfod ipv4-ipv6 dual-stack From: Mark Wielaard To: "Frank Ch. Eigler" , elfutils-devel@sourceware.org Date: Mon, 04 Apr 2022 18:58:12 +0200 In-Reply-To: <20220403234943.GB933@redhat.com> References: <20220403234943.GB933@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Evolution 3.28.5 (3.28.5-10.el7) Mime-Version: 1.0 X-Spam-Status: No, score=-10.0 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: elfutils-devel@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Elfutils-devel mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Apr 2022 16:58:20 -0000 Hi Frank, On Sun, 2022-04-03 at 19:49 -0400, Frank Ch. Eigler via Elfutils-devel wrot= e: > commit 3a00f412b6554ba70f7b7e3d3aa6f67f278868af (HEAD -> master) > Author: Frank Ch. Eigler > Date: Sun Apr 3 19:42:48 2022 -0400 >=20 > debuginfod: use single ipv4+ipv6 microhttpd daemon configuration > =20 > Use a single MHD_USE_DUAL_STACK mhd daemon. This way, the thread > connection pool is not doubled, saving memory and better matching use= r > 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. > =20 > Signed-off-by: Frank Ch. Eigler >=20 > 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 > + > + * 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 > =20 > * debuginfod.cxx (main): Use MHD_USE_EPOLL for libmicrohttpd, to > diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-clien= t.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 *cli= ent, > int debuginfod_add_http_header (debuginfod_client *client, const char* h= eader) > { > /* 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 =3D strchr (header, ':'); > - if (colon =3D=3D NULL > - || colon =3D=3D header > - || *(colon + 1) =3D=3D '\0' > - || strchr (colon + 1, ':') !=3D NULL) > + char *colon =3D strchr (header, ':'); /* first colon */ > + if (colon =3D=3D NULL /* present */ > + || colon =3D=3D header /* not at beginning - i.e., have a header n= ame */ > + || *(colon + 1) =3D=3D '\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 =3D 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 =3D getnameinfo (so, sizeof (struct sockaddr_in), hostname, size= of (hostname), servname, > sizeof (servname), NI_NUMERICHOST | NI_NUMERICSER= V); > } else if (so && so->sa_family =3D=3D AF_INET6) { > - sts =3D getnameinfo (so, sizeof (struct sockaddr_in6), hostname, siz= eof (hostname), > - servname, sizeof (servname), NI_NUMERICHOST | NI_= NUMERICSERV); > + struct sockaddr_in6* addr6 =3D (struct sockaddr_in6*) so; > + if (IN6_IS_ADDR_V4MAPPED(&addr6->sin6_addr)) { > + struct sockaddr_in addr4; > + memset (&addr4, 0, sizeof(addr4)); > + addr4.sin_family =3D AF_INET; > + addr4.sin_port =3D addr6->sin6_port; > + memcpy (&addr4.sin_addr.s_addr, addr6->sin6_addr.s6_addr+12, sizeo= f(addr4.sin_addr.s_addr)); > + sts =3D getnameinfo ((struct sockaddr*) &addr4, sizeof (addr4), > + hostname, sizeof (hostname), servname, sizeof (= servname), > + NI_NUMERICHOST | NI_NUMERICSERV); > + } else { > + sts =3D getnameinfo (so, sizeof (struct sockaddr_in6), hostname, s= izeof (hostname), NULL, 0, > + NI_NUMERICHOST); > + } > } That IN6_IS_ADDR_V4MAPPED is funny. Is that actually used in practice? > if (sts !=3D 0) { > hostname[0] =3D servname[0] =3D '\0'; > } > @@ -2008,13 +2021,26 @@ and will not query the upstream servers"); > M= HD_CONNECTION_INFO_CLIENT_ADDRESS); > struct sockaddr *so =3D u ? u->client_addr : 0; > char hostname[256] =3D ""; // RFC1035 > - if (so && so->sa_family =3D=3D AF_INET) > + if (so && so->sa_family =3D=3D AF_INET) { > (void) getnameinfo (so, sizeof (struct sockaddr_in), hostnam= e, sizeof (hostname), NULL, 0, > NI_NUMERICHOST); > - else if (so && so->sa_family =3D=3D AF_INET6) > - (void) getnameinfo (so, sizeof (struct sockaddr_in6), hostna= me, sizeof (hostname), NULL, 0, > - NI_NUMERICHOST); > - > + } else if (so && so->sa_family =3D=3D AF_INET6) { > + struct sockaddr_in6* addr6 =3D (struct sockaddr_in6*) so; > + if (IN6_IS_ADDR_V4MAPPED(&addr6->sin6_addr)) { > + struct sockaddr_in addr4; > + memset (&addr4, 0, sizeof(addr4)); > + addr4.sin_family =3D AF_INET; > + addr4.sin_port =3D addr6->sin6_port; > + memcpy (&addr4.sin_addr.s_addr, addr6->sin6_addr.s6_addr+1= 2, sizeof(addr4.sin_addr.s_addr)); > + (void) getnameinfo ((struct sockaddr*) &addr4, sizeof (add= r4), > + hostname, sizeof (hostname), NULL, 0, > + NI_NUMERICHOST); > + } else { > + (void) getnameinfo (so, sizeof (struct sockaddr_in6), host= name, sizeof (hostname), NULL, 0, > + NI_NUMERICHOST); > + } > + } Too bad this cannot be merged with conninfo above. But this calculates less info. =20 > string xff_complete =3D 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[]) > } > } > =20 > - // Start httpd server threads. Separate pool for IPv4 and IPv6, in > - // case the host only has one protocol stack. > - MHD_Daemon *d4 =3D MHD_start_daemon ((connection_pool ? 0 : MHD_USE_TH= READ_PER_CONNECTION) > -#if MHD_VERSION >=3D 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 t= o stderr */ > - http_port, > - NULL, NULL, /* default accept polic= y */ > - handler_cb, NULL, /* handler callba= ck */ > - MHD_OPTION_EXTERNAL_LOGGER, error_c= b, NULL, > - (connection_pool ? MHD_OPTION_THREA= D_POOL_SIZE : MHD_OPTION_END), > - (connection_pool ? (int)connection_= pool : MHD_OPTION_END), > - MHD_OPTION_END); > - MHD_Daemon *d6 =3D MHD_start_daemon ((connection_pool ? 0 : MHD_USE_TH= READ_PER_CONNECTION) > + // Start httpd server threads. Use a single dual-homed pool. > + MHD_Daemon *d46 =3D MHD_start_daemon ((connection_pool ? 0 : MHD_USE_T= HREAD_PER_CONNECTION) > #if MHD_VERSION >=3D 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 t= o stderr */ > http_port, > NULL, NULL, /* default accept polic= y */ Nice cleanup. > @@ -3911,7 +3919,7 @@ main (int argc, char *argv[]) > (connection_pool ? (int)connection_= pool : MHD_OPTION_END), > MHD_OPTION_END); > =20 > - if (d4 =3D=3D NULL && d6 =3D=3D NULL) // neither ipv4 nor ipv6? boo > + if (d46 =3D=3D NULL) > { > sqlite3 *database =3D db; > sqlite3 *databaseq =3D dbq; > @@ -3922,8 +3930,8 @@ main (int argc, char *argv[]) > } > =20 > obatched(clog) << "started http server on " > - << (d4 !=3D NULL ? "IPv4 " : "") > - << (d6 !=3D NULL ? "IPv6 " : "") > + << "IPv4 " > + << "IPv6 " > << "port=3D" << http_port << endl; This keeps the log output the same, but I wouldn't mind just removing the IPv4 and IPv6 strings. Cheers, Mark