patch: debuginfod ipv4-ipv6 dual-stack

  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>
  

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
  

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