Bug#682342: Latest patch successfully tested

[Nishanth Aravamudan]

On 22.08.2019 [21:45:26 +0200], Philipp Kern wrote:
> On 8/16/2019 8:34 PM, Nishanth Aravamudan wrote:
> > On 15.08.2019 [17:08:39 +0200], Cyril Brulebois wrote:
> >> Nishanth Aravamudan  (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.c2017-10-10 14:01:42.0 +
> > +++ b/dhcp.c2019-08-14 01:04:58.339325357 +
> > @@ -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.0 +
> > +++ b/netcfg-common.c   2019-08-13 20:01:13.606510273 +
> > @@ -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.

Thank you, fixed below, I believe.

diff -Naur a/dhcp.c b/dhcp.c
--- a/dhcp.c2017-10-10 14:01:42.0 +
+++ b/dhcp.c2019-08-14 01:04:58.339325357 +
@@ -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.0 +
+++ b/netcfg-common.c   2019-08-13 20:01:13.606510273 +
@@ -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 

Bug#682342: Latest patch successfully tested

[Philipp Kern]

On 8/16/2019 8:34 PM, Nishanth Aravamudan wrote:
> On 15.08.2019 [17:08:39 +0200], Cyril Brulebois wrote:
>> Nishanth Aravamudan  (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.0 +
> +++ b/dhcp.c  2019-08-14 01:04:58.339325357 +
> @@ -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.0 +
> +++ b/netcfg-common.c 2019-08-13 20:01:13.606510273 +
> @@ -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

Bug#682342: Latest patch successfully tested

[Nishanth Aravamudan]

On 15.08.2019 [17:08:39 +0200], Cyril Brulebois wrote:
> Hi,
> 
> Nishanth Aravamudan  (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.c2017-10-10 14:01:42.0 +
+++ b/dhcp.c2019-08-14 01:04:58.339325357 +
@@ -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.0 +
+++ b/netcfg-common.c   2019-08-13 20:01:13.606510273 +
@@ -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);
+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.

-Nish

Bug#682342: Latest patch successfully tested

[Cyril Brulebois]

Hi,

Nishanth Aravamudan  (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? :)

I'm a little reluctant to blindly merging this patch (originally
labeled “untested”) without a go from its author. Philipp, should
I go ahead?


Cheers,
-- 
Cyril Brulebois (k...@debian.org)
D-I release manager -- Release team member -- Freelance Consultant


signature.asc
Description: PGP signature

Bug#682342: Latest patch successfully tested

[Nishanth Aravamudan]

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!
-Nish