(In reply to Francesco Giudici from comment #0)
> The ifcfg-rh plugin binds both ipv4.dhcp-hostname and ipv6.dhcp-hostname
> properties to the same DHCP_HOSTNAME ifcfg VAR.
> ipv6.dhcp-hostname is saved only if ipv6.method is "dhcp": in this case will
> overwrite the value in ipv4.dhcp-hostname (if present).
> When reading the ifcfg file instead, ipv6.dhcp-hostname is filled with the
> value from DHCP_HOSTNAME if ipv6.method is "dhcp" or "auto".
> 
> As ifcfg scripts does not deal with DHCP_HOSTNAME for IPv6, leave
> DHCP_HOSTNAME for IPv4 and add a new, NM only VAR for ipv6.dhcp-hostname
> (DHCP_HOSTNAMEv6 ?).

DHCPV6_HOSTNAME ^^^

* Added two new ifcfg vars: DHCPV6_HOSTNAME and DHCPV6_SEND_HOSTNAME
* Preserve old ifcfg files, loading ipv6.dhcp-hostname from DHCP_HOSTNAME when:
  - no DHCPV6_HOSTNAME is available
  AND
  - DHCP_HOSTNAME contains a FQDN
* Added ifcfg test

Pushed to branch fg/dhcp_hostname_bgo775622, please review

+    if (hostname)
+         svSetValueString (ifcfg, "DHCPV6_HOSTNAME", hostname);
+    else
+         svUnsetValue (ifcfg, "DHCPV6_HOSTNAME");

svSetValueString with hostname=NULL, is equal to svUnsetValue. The if() is not necessary.



+         value = svGetValueString (ifcfg, "DHCPV6_HOSTNAME");
          if (value && value[0])

svGetValueString() *never* returns an empty string "" -- which is precisely the difference to svGetValue(). Let's drop the check for value[0].



>> ifcfg-rh: add ipv6 hostname legacy support
    
doesn't this commit just fix the previous commit from the same branch? If yes, they probably should be squashed together. Sometimes I would keep such commits, if they are by different authors, but that's not the case here.

(In reply to Thomas Haller from comment #3)
> +    if (hostname)
> +         svSetValueString (ifcfg, "DHCPV6_HOSTNAME", hostname);
> +    else
> +         svUnsetValue (ifcfg, "DHCPV6_HOSTNAME");
> 
> svSetValueString with hostname=NULL, is equal to svUnsetValue. The if() is
> not necessary.
> 

Fixed, thanks

> 
> 
> +         value = svGetValueString (ifcfg, "DHCPV6_HOSTNAME");
>           if (value && value[0])
> 
> svGetValueString() *never* returns an empty string "" -- which is precisely
> the difference to svGetValue(). Let's drop the check for value[0].
> 

Fixed

> 
> 
> >> ifcfg-rh: add ipv6 hostname legacy support
>     
> doesn't this commit just fix the previous commit from the same branch? If
> yes, they probably should be squashed together. Sometimes I would keep such
> commits, if they are by different authors, but that's not the case here.

The reason behind having it as a separate commit was to highlight the "fallback" code to pick DHCPv6 FQDN from DHCP_HOSTNAME if in FQDN format. This is required to support old config files that could have stored the IPv6 FQDN there. But after all it is just three lines "fixing" the previous commit as you pointed out: squashed.

Repushed on fg/dhcp_hostname_bgo775622.
Old code moved to fg/dhcp_hostname_old.

Thanks

looking at libnm-core, it shows that dhcp_send_hostname and and dhcp_hostname can be set for every method. That is, NMSettingIPxConfig:verify() does not reject setting those properties for a method != AUTO/DHCP.

The ifcfg-rh reader/writer should be able to *fully* deserialize/serialize a connection. Since, setting hostname with method=ignored is not invalid, ifcfg must support that too.

Alternatively, normalize() should fix such connections.

(In reply to Thomas Haller from comment #5)
> looking at libnm-core, it shows that dhcp_send_hostname and and
> dhcp_hostname can be set for every method. That is,
> NMSettingIPxConfig:verify() does not reject setting those properties for a
> method != AUTO/DHCP.
> 
> The ifcfg-rh reader/writer should be able to *fully* deserialize/serialize a
> connection. Since, setting hostname with method=ignored is not invalid,
> ifcfg must support that too.
> 
> Alternatively, normalize() should fix such connections.

I'm for accepting the two properties for all methods in ifcfg-rh.

Other than that, the branch LGTM.

(In reply to Beniamino Galvani from comment #6)
> (In reply to Thomas Haller from comment #5)
> > looking at libnm-core, it shows that dhcp_send_hostname and and
> > dhcp_hostname can be set for every method. That is,
> > NMSettingIPxConfig:verify() does not reject setting those properties for a
> > method != AUTO/DHCP.
> > 
> > The ifcfg-rh reader/writer should be able to *fully* deserialize/serialize a
> > connection. Since, setting hostname with method=ignored is not invalid,
> > ifcfg must support that too.
> > 
> > Alternatively, normalize() should fix such connections.
> 
> I'm for accepting the two properties for all methods in ifcfg-rh.
> 
> Other than that, the branch LGTM.


ipv4.dhcp-client-id, ipv4.fqdn, ipvX.ignore-auto-routes, ipvX.ignore-auto-dns and ipv4.dhcp-timeout properties suffer of the same issue.

I don't see any cons to let save/restore these properties also when they are not used (no dhcp conf in place). So removed the check on method for all of them.
Updated branch fg/dhcp_hostname_bgo775622.
Previous branch version saved on fg/dhcp_hostname_old.


IMO a more cleaner implementation would be to have the dhcp-hostname property in IPv4 only and the fqdn one both in IPv6 and IPv4 (we use dhcp-hostname in IPv6 as fqdn option). Maybe having fqdn property also shared would be better.
But such a rework would be too disrupting, isn't it? So, better to stick with current behavior:

ipv4.dhcp-hostname  --  DHCPv4 option 12 "Host Name", see RFC 2132
ipv4.dhcp-fqdn -- DHCPv4 option 81 "client FQDN", see RFC 4702
ipv6.dhcp-hostname -- DHCPv6 option 39 "client FQDN", see RFC 4704

lgtm

Thanks, merged branch fg/dhcp_hostname_bgo775622 upstream:

https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=2ee6b7bf