Bug 1445414

Created attachment 1274362 [details]
Proposed patch

Created attachment 1274379 [details]
[patch] ifcfg-rh: treat a wrongly quoted value like empty string

src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c:2107:7: error: variable 'tmp' is used uninitialized whenever '||' condition is true [-Werror,-Wsometimes-uninitialized]
                if (!num || g_strcmp0 (svGetValue (ifcfg, "BOOTPROTO", &tmp), "static"))





+         tmp = g_strdup_printf ("%u", prefix);
          svSetValueStr (ifcfg, prefix_key, tmp);
          g_free (tmp);

svSetValueInt64()?


+         tmp = svGetValueStr_cp (ifcfg, netmask_key);
          svUnsetValue (ifcfg, netmask_key);
+         if (tmp) {

the "_cp" stands for copy. Using it is IMO dicouraged, because you can avoid the copy in many cases.

Also, svGetValueStr() swallows empty string (often useful where a shell script might do `test -n`). It seems in this case, the svGetValue() would be more appropriate.


  if (inet_ntop (AF_INET, &prefix, buf, INET_ADDRSTRLEN))

inet_ntop() cannot fail (unless you provide an invalid family, which you do not). Also, more often we use nm_utils_inet4_ntop().





Together with patch 0001-ifcfg-rh-treat-a-wrongly-quoted-value-like-empty-str.patch, it would be:

  if (svGetValue (ifcfg, netmask_key, &tmp) {
      char buf[INET_ADDRSTRLEN];

      g_free (tmp);
      svSetValueStr (ifcfg, netmask_key,
                     nm_utils_inet4_ntop(prefix, buf));
  }

merged upstream:

master: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=9e668595fee2127c20310f50e405cd3450f88f5b

nm-1-8: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=d21b21eba152152b067501673098a5bea82ae5c7

This doesn't look good. NETMASK was chnaged to PREFIX. I think this shouldn't happen.

    [root@qe-dell-ovs5-vm-6 NetworkManager]# cat /etc/sysconfig/network-scripts/ifcfg-eth1
    # Generated by parse-kickstart
    UUID="8b4753fb-c562-4784-bfa7-f44dc6581e73"
    DNS1="192.0.2.1"
    IPADDR="102.0.2.2"
    GATEWAY="192.0.2.1"
    NETMASK="255.255.255.0"
    BOOTPROTO="static"
    DEVICE="eth1"
    ONBOOT="yes"
    IPV6INIT="yes"
    [root@qe-dell-ovs5-vm-6 NetworkManager]# nmcli connection modify "System eth1" ipv4.addresses 1.2.3.4/24 ipv4.gateway 1.2.3.1
    [root@qe-dell-ovs5-vm-6 NetworkManager]# nmcli connection show "System eth1"
    connection.id:                          System eth1
    connection.uuid:                        8b4753fb-c562-4784-bfa7-f44dc6581e73
    connection.stable-id:                   --
    connection.interface-name:              eth1
    connection.type:                        802-3-ethernet
    connection.autoconnect:                 yes
    connection.autoconnect-priority:        0
    connection.autoconnect-retries:         -1 (default)
    connection.timestamp:                   1495630993
    connection.read-only:                   no
    connection.permissions:                 --
    connection.zone:                        --
    connection.master:                      --
    connection.slave-type:                  --
    connection.autoconnect-slaves:          -1 (default)
    connection.secondaries:                 --
    connection.gateway-ping-timeout:        0
    connection.metered:                     unknown
    connection.lldp:                        -1 (default)
    802-3-ethernet.port:                    --
    802-3-ethernet.speed:                   0
    802-3-ethernet.duplex:                  --
    802-3-ethernet.auto-negotiate:          no
    802-3-ethernet.mac-address:             --
    802-3-ethernet.cloned-mac-address:      --
    802-3-ethernet.generate-mac-address-mask:--
    802-3-ethernet.mac-address-blacklist:   --
    802-3-ethernet.mtu:                     auto
    802-3-ethernet.s390-subchannels:        --
    802-3-ethernet.s390-nettype:            --
    802-3-ethernet.s390-options:            --
    802-3-ethernet.wake-on-lan:             1 (default)
    802-3-ethernet.wake-on-lan-password:    --
    ipv4.method:                            manual
    ipv4.dns:                               192.0.2.1
    ipv4.dns-search:                        --
    ipv4.dns-options:                       (default)
    ipv4.dns-priority:                      0
    ipv4.addresses:                         1.2.3.4/24
    ipv4.gateway:                           1.2.3.1
    ipv4.routes:                            { ip = 1.2.3.1/32 }
    ipv4.route-metric:                      -1
    ipv4.ignore-auto-routes:                no
    ipv4.ignore-auto-dns:                   no
    ipv4.dhcp-client-id:                    --
    ipv4.dhcp-timeout:                      0
    ipv4.dhcp-send-hostname:                yes
    ipv4.dhcp-hostname:                     --
    ipv4.dhcp-fqdn:                         --
    ipv4.never-default:                     no
    ipv4.may-fail:                          yes
    ipv4.dad-timeout:                       -1 (default)
    ipv6.method:                            auto
    ipv6.dns:                               --
    ipv6.dns-search:                        --
    ipv6.dns-options:                       (default)
    ipv6.dns-priority:                      0
    ipv6.addresses:                         --
    ipv6.gateway:                           --
    ipv6.routes:                            --
    ipv6.route-metric:                      -1
    ipv6.ignore-auto-routes:                no
    ipv6.ignore-auto-dns:                   no
    ipv6.never-default:                     no
    ipv6.may-fail:                          yes
    ipv6.ip6-privacy:                       -1 (unknown)
    ipv6.addr-gen-mode:                     eui64
    ipv6.dhcp-send-hostname:                yes
    ipv6.dhcp-hostname:                     --
    ipv6.token:                             --
    proxy.method:                           none
    proxy.browser-only:                     no
    proxy.pac-url:                          --
    proxy.pac-script:                       --
    [root@qe-dell-ovs5-vm-6 NetworkManager]# cat /etc/sysconfig/network-scripts/ifcfg-eth1
    # Generated by parse-kickstart
    UUID=8b4753fb-c562-4784-bfa7-f44dc6581e73
    DNS1=192.0.2.1
    IPADDR=1.2.3.4
    GATEWAY=1.2.3.1
    BOOTPROTO="static"
    DEVICE=eth1
    ONBOOT=yes
    IPV6INIT=yes
    TYPE=Ethernet
    PROXY_METHOD=none
    BROWSER_ONLY=no
    PREFIX=24
    DEFROUTE=yes
    IPV4_FAILURE_FATAL=no
    IPV6_AUTOCONF=yes
    IPV6_DEFROUTE=yes
    IPV6_FAILURE_FATAL=no
    NAME="System eth1"

fix for master: th/ifcfg-netmask-legacy-rh1445414

(as the code changed there, the backport for nm-1-8 will need some adjustments).

Please review.

(In reply to Thomas Haller from comment #7)
> fix for master: th/ifcfg-netmask-legacy-rh1445414

LGTM

Branch LGTM.

In the message of "ifcfg-rh: fix preserving NETMASK key in write_ip4_setting()" commit I would explicitly state that also if we preserve the legacy NETMASK key there we will add also the PREFIX key. That is not clear.

In the same commit message minor typo in the second paragraph:
"That was borken [...]"
"That was bROken [...]"

fixed upstream:

master: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=70c0f7965d0d028364a5de05113974880e829706


the branch was partially backported to nm-1-8:
https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=47ebca23e3403243a97c1d0936f023a7c9f1ac38


... and will be fixed in next rhel-7.4 package.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2017:2299