Bug#682342: Latest patch successfully tested

[Philipp Kern <pkern@debian.org>]

On 8/16/2019 8:34 PM, Nishanth Aravamudan wrote:
> On 15.08.2019 [17:08:39 +0200], Cyril Brulebois wrote:
>> Nishanth Aravamudan <naravamudan@digitalocean.com> (2019-08-14):
>>> We are able to reproduce this issue at will in Ubuntu Bionic's
>>> installer (not identical to Debian's, but code-wise in this path the
>>> same).  While quite a while after the last update from Philipp, we
>>> tested the patch (netcfg_dhcp_domain.patch) after updating it to avoid
>>> a compilation issue, we found it did fix the problem for us.
>>>
>>> I am not sure if I can get Debian into our infrastructure to test
>>> explicitly, but I will work on it; at the same time,  the code change
>>> seems straightforward.
>>
>> Thanks for your feedback. Care to share the fixed version? :)
> 
> D'oh! I'm sorry, I thought I did. The patch we tested was:
> 
> diff -Naur a/dhcp.c b/dhcp.c
> --- a/dhcp.c	2017-10-10 14:01:42.000000000 +0000
> +++ b/dhcp.c	2019-08-14 01:04:58.339325357 +0000
> @@ -590,7 +590,7 @@
>                          preseed_hostname_from_fqdn(client, buf);
>                  }
>  
> -                if (netcfg_get_hostname (client, "netcfg/get_hostname", hostname, 1)) {
> +                if (netcfg_get_hostname (client, "netcfg/get_hostname", hostname, !have_domain)) {
>                      /*
>                       * Going back to POLL wouldn't make much sense.
>                       * However, it does make sense to go to the retry
> diff -Naur a/netcfg-common.c b/netcfg-common.c
> --- a/netcfg-common.c	2017-10-10 14:04:08.000000000 +0000
> +++ b/netcfg-common.c	2019-08-13 20:01:13.606510273 +0000
> @@ -1060,14 +1060,24 @@
>              continue;
>          }
>  
> -        if (accept_domain && (s = strchr(hostname, '.'))) {
> -            di_info("Detected we have an FQDN; splitting and setting domain");
> -            if (s[1] == '\0') { /* "somehostname." <- . should be ignored */
> +        if ((s = strchr(hostname, '.'))) {
> +            di_info("Detected an FQDN in hostname");
> +            if (s[1] == '\0') {
> +                /* "somehostname." <- . should be ignored */
>                  *s = '\0';
> -            } else { /* assume we have a valid domain name given */
> -                strncpy(domain, s + 1, MAXHOSTNAMELEN);
> -                debconf_set(client, "netcfg/get_domain", domain);
> -                have_domain = 1;
> +                di_info("Stripped trailing dot from hostname");
> +            } else {
> +                /* assume that the domain is valid and copy it if
> +                 * accept_domain is set; just use the hostname if
> +                 * it is unset
> +                 */
> +                if (accept_domain) {
> +                    strncpy(domain, s + 1, MAXHOSTNAMELEN);
> +					di_info("Setting domain to %s", domain);

This needs indenting fix-up.

> +                    debconf_set(client, "netcfg/get_domain", domain);
> +                    have_domain = 1;
> +                }
> +                /* strip the domain from the hostname */
>                  *s = '\0';
>              }
>          }
> 
>> I'm a little reluctant to blindly merging this patch (originally
>> labeled “untested”) without a go from its author. Philipp, should
>> I go ahead?
> 
> Totally understood! I just wanted to make sure to revive this issue, as
> I'd also like to get it fixed in Ubuntu! Like I said, I will do my best
> to test and reproduce the fix with stock Debian.

I think this should be fine and we're early in the release cycle to find
potential problems if there are any.

Obviously it'd be great to have a test hardness with a DHCP server
sending various bits and us verifying that netcfg did the right thing.
But I'd surprised to find the time for that myself.

Kind regards and thanks
Philipp Kern