[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH 07/22] Use ifcfg files via NetworkDevice in Network class (#520146)



David Cantrell wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Comments below.

On Tue, 27 Apr 2010, Radek Vykydal wrote:

Remove reading of some keys from NM via dbus, count on ifcfg
file contents (written by stage 1, ks, or nm-c-e). This should
also ensure support for ipv6 in stage2 using nm-c-e.

Remove available() method - it is useless - HWADDR is read from ifcfg
file, description is set in constructor, and Network class will be
kept up-to-date (e.g. after configuring with nm-c-e) by explicit
call to added update() method.

readIfcfgContets became useless, we can use (now ifcfg-backed)
NetworkDevices' method loadIfcfgFile instead.

Hunk starting with if len(available_devices) > 0: is a noop,
probably some leftover - removing.
---
iw/advanced_storage.py  |    2 +-
network.py | 159 ++++++++++------------------------------------
textw/add_drive_text.py |    2 +-
textw/netconfig_text.py |    4 +-
textw/partition_text.py |    1 +
5 files changed, 40 insertions(+), 128 deletions(-)

diff --git a/iw/advanced_storage.py b/iw/advanced_storage.py
index d3d4360..c03b3ab 100644
--- a/iw/advanced_storage.py
+++ b/iw/advanced_storage.py
@@ -43,7 +43,7 @@ def addFcoeDrive(anaconda):
    store = gtk.TreeStore(gobject.TYPE_STRING, gobject.TYPE_STRING)
    combo.set_model(store)

-    netdevs = anaconda.network.available()
+    netdevs = anaconda.network.netdevices
    keys = netdevs.keys()
    keys.sort()
    selected_interface = None
diff --git a/network.py b/network.py
index 20a21b2..14af8c8 100644
--- a/network.py
+++ b/network.py
@@ -315,145 +315,56 @@ class NetworkDevice(IfcfgFile):


class Network:
+
    def __init__(self):
+
+        self.hostname = socket.gethostname()
+        self.overrideDHCPhostname = False
+        self.update()
+
+    def update(self):
+
        self.netdevices = {}
        self.ksdevice = None
        self.domains = []
-        self.hostname = socket.gethostname()
-        self.overrideDHCPhostname = False

See below.


        # populate self.netdevices
        devhash = isys.getDeviceProperties(dev=None)
-        for dev in devhash.keys():
-            self.netdevices[dev] = NetworkDevice(dev)
-            ifcfg_contents = self.readIfcfgContents(dev)
-
-            # if NM_CONTROLLED is set to yes, we read in settings from
-            # NetworkManager first, then fill in the gaps with the data
-            # from the ifcfg file
-            useNetworkManager = False
-            if ifcfg_contents.has_key('NM_CONTROLLED') and \
-               not ifcfg_contents['NM_CONTROLLED'].lower() == 'no':
-                useNetworkManager = True
-
-            # this interface is managed by NetworkManager, so read from
-            # NetworkManager first
-            if useNetworkManager:
-                props = devhash[dev]
-
-                if isys.isDeviceDHCP(dev):
-                    self.netdevices[dev].set(('BOOTPROTO', 'dhcp'))
-                else:
-                    self.netdevices[dev].unset('BOOTPROTO')
-                    bus = dbus.SystemBus()
- config_path = props.Get(isys.NM_MANAGER_IFACE, 'Ip4Config') - config = bus.get_object(isys.NM_SERVICE, config_path) - config_props = dbus.Interface(config, isys.DBUS_PROPS_IFACE)
-
- # addresses (3-element list: ipaddr, netmask, gateway) - addrs = config_props.Get(isys.NM_MANAGER_IFACE, 'Addresses')[0]
-                    try:
-                        tmp = struct.pack('I', addrs[0])
-                        ipaddr = socket.inet_ntop(socket.AF_INET, tmp)
-                        self.netdevices[dev].set(('IPADDR', ipaddr))
-                    except:
-                        pass
-
-                    try:
-                        tmp = struct.pack('I', addrs[1])
-                        netmask = socket.inet_ntop(socket.AF_INET, tmp)
-                        self.netdevices[dev].set(('NETMASK', netmask))
-                    except:
-                        pass
-
-                    try:
-                        tmp = struct.pack('I', addrs[2])
-                        gateway = socket.inet_ntop(socket.AF_INET, tmp)
-                        self.netdevices[dev].set(('GATEWAY', gateway))
-                    except:
-                        pass
-
-                self.hostname = socket.gethostname()
-
-            # read in remaining settings from ifcfg file
-            for key in ifcfg_contents.keys():
-                if key == 'GATEWAY':
- self.netdevices[dev].set((key, ifcfg_contents[key]))
-                elif key == 'DOMAIN':
-                    self.domains.append(ifcfg_contents[key])
-                elif key == 'HOSTNAME':
-                    self.hostname = ifcfg_contents[key]
-                elif self.netdevices[dev].get(key) == '':
- self.netdevices[dev].set((key, ifcfg_contents[key]))
-
-        # now initialize remaining devices
-        # XXX we just throw return away, the method initialize a
-        # object member so we dont need to
-        available_devices = self.available()
-
-        if len(available_devices) > 0:
-            # set first device to start up onboot
-            oneactive = 0
-            for dev in available_devices.keys():
-                try:
-                    if available_devices[dev].get("ONBOOT") == "yes":
-                        oneactive = 1
-                        break
-                except:
-                    continue
+        for iface in devhash.keys():
+ device = NetworkDevice(netscriptsDir, iface, logfile=ifcfgLogFile)
+            device.loadIfcfgFile()
+            device.log("===== Network.update\n")

Just concerns...

The concern I have with eliminating this loop is for upgrade mode and rescue mode, but maybe we have eliminated this code path for those modes. I do not know. Based on your other patches in this series, handling interfaces that are NM_CONTROLLED 'yes' or 'no' seems pointless if NM can use the ifcfg file. D-Bus is, in theory, the preferred way to talk to NetworkManager. However, since the move to using NetworkManager in the installer, the D-Bus API has been extremely unreliable, so I have no problems eliminating our usage of it
here and relying on the ifcfg file contents instead.

I think that to set values we already talk to NM exclusively through ifcfg files,
so using them to read the vaues seem to be right to me. And with nm-c-e, we
don't have control over what was or can be configured, so let's rely on ifcfg files
that nm-c-e updates for us and use them as they are, otherwise we can easily
miss something.

I'll check that we can use this approach also in upgrade and rescue safely.


-    def readIfcfgContents(self, dev):
-        ifcfg = "/etc/sysconfig/network-scripts/ifcfg-%s" % (dev,)
-        contents = {}
+            if device.get('DOMAIN'):
+                self.domains.append(device.get('DOMAIN'))
+            # TODORV - the last iface in loop wins, might be ok,
+            #          not worthy of special juggling
+            if device.get('HOSTNAME'):
+                self.hostname = device.get('HOSTNAME')

The HOSTNAME value in /etc/sysconfig/network should always override HOSTNAME from an ifcfg file. Likewise, if we can do a reverse DNS lookup and get a
hostname, we should use that if the ifcfg file and network file did not
provide us with one.

The behavior has not changed in the patch (see line 384 in original network.py).



-        try:
-            f = open(ifcfg, "r")
-            lines = f.readlines()
-            f.close()
+            device.description = isys.getNetDevDesc(iface)

-            for line in lines:
-                line = line.strip()
-                if line.startswith('#') or line == '':
-                    continue
+            self.netdevices[iface] = device

-                var = string.splitfields(line, '=', 1)
-                if len(var) == 2:
-                    var[1] = var[1].replace('"', '')
-                    contents[var[0]] = string.strip(var[1])
-        except:
-            return {}
-
-        return contents
-
-    def getDevice(self, device):
-        return self.netdevices[device]
-
-    def available(self):
-        ksdevice = None
-        if flags.cmdline.has_key('ksdevice'):
-            ksdevice = flags.cmdline['ksdevice']
-
-        for dev in isys.getDeviceProperties().keys():
-            if not self.netdevices.has_key(dev):
-                self.netdevices[dev] = NetworkDevice(dev)
-
-            hwaddr = isys.getMacAddress(dev)

-            self.netdevices[dev].set(('HWADDR', hwaddr))
-            self.netdevices[dev].set(('DESC', isys.getNetDevDesc(dev)))
+        ksdevice = flags.cmdline.get('ksdevice', None)
+        if ksdevice:
+            for dev in self.netdevices:
+                if ksdevice == 'link' and isys.getLinkStatus(dev):
+                    self.ksdevice = dev
+                    break
+                elif ksdevice == dev:
+                    self.ksdevice = dev
+                    break
+                elif ':' in ksdevice:
+ if ksdevice.upper() == self.netdevices[dev].get('HWADDR'):
+                        self.ksdevice = dev
+                        break

-            if not ksdevice:
-                continue

-            if ksdevice == 'link' and isys.getLinkStatus(dev):
-                self.ksdevice = dev
-            elif ksdevice == dev:
-                self.ksdevice = dev
-            elif ksdevice.find(':') != -1:
-                if ksdevice.upper() == hwaddr:
-                    self.ksdevice = dev

The available() method is meant to give us a list of usable interfaces during installation, not of all interfaces. Are you accounting for that elsewhere?

Yes, name of available() would suggest meaning you describe, but what it
did in fact (as far as I am able to read it) is that it only:

1) added device to Network object if it found a new one ... I don't
see the meaning of this
2) updated HWADDR and DESC - these values are set in Network object
initialization in the patch (HWADDR from ifcfg file written in stage 1,
description/DESC is obtained in the same way as in available())
3) set ksdevice ... this is (and was before tha patch) done already in Network object init in the patch, ksdevice is used only for device preselection in UI. The only thing that would make difference when setting it again in available()
is change of getLinkStatus for ksdevice=='link' but it does not make
much sense to me (better preselection after connecting a cable?).

It seems to me that the method is leftover of something that might have
been useful before, but now I don't think it really does anything that
would justify its existence.

Things like the advanced network storage configuration dialog or the add repo dialog present a list of usable network devices, but on the network screen in anaconda we do want people to be able to configure all network interfaces in
the system and not just the ones we have found to be usable during
installation.


Both network enablement (adv. net storage, add repo cases) and network
configuration (the network screen case) use nm-c-e.

1) In configuration, it makes by default all devices that could be edited by
nm-c-e (that is devs having ifcfg file?) present in nm-c-e by prechecking
them in dialog preceding nm-c-e itself (the mechanism is that for the
checked ones we set NM_CONTROLLED to yes before invoking nm-c-e)

like

+--------------------------------+
| Select which devices should be |
| configured with NetworkManager |
| [+] eth0                       |
| [+] wlan0                      |
| [+] XXX                        |
+--------------------------------+


2) In enablement, all devices are prechecked as controlled by NM too,
but the dialog contains also combobox to select one device to be used
for installation - this device will have set ONBOOT to yes before calling
nm-c-e (and therefore "Connect automatically" prechecked in nm-c-e).

+-----------------------------------------------+
|This requires that you have an active          |
|network connection during the installation     |
|process. Please configure a network interface. |
|  +---------------------------------------+    |
|  | eth0 - description - hwaddress        |    |
|  +---------------------------------------+    |
|                                               |
| Select which devices should be                |
| configured with NetworkManager                |
| [+] eth0                                      |
| [+] wlan0                                     |
| [+] XXX                                       |
+-----------------------------------------------+


Now to make the difference you suggest, we could filter out (not precheck,
gray-out?) some devices in enablement (e.g. NetworkStorageDevices?) or even
get rid of lower part of dialog and make only device selected in combobox visible
in nm-c-e. It should be easily doable.

-        return self.netdevices
+    def getDevice(self, device):
+        return self.netdevices[device]

    def getKSDevice(self):
        if self.ksdevice is None:
diff --git a/textw/add_drive_text.py b/textw/add_drive_text.py
index 0fbd86c..663d45d 100644
--- a/textw/add_drive_text.py
+++ b/textw/add_drive_text.py
@@ -78,7 +78,7 @@ class addDriveDialog(object):
        return INSTALL_OK

    def addFcoeDriveDialog(self, screen):
-        netdevs = self.anaconda.network.available()
+        netdevs = self.anaconda.network.netdevices
        devs = netdevs.keys()
        devs.sort()

diff --git a/textw/netconfig_text.py b/textw/netconfig_text.py
index 4cea192..fb37429 100644
--- a/textw/netconfig_text.py
+++ b/textw/netconfig_text.py
@@ -96,7 +96,7 @@ class NetworkConfiguratorText:

        self.interfaceList = CheckboxTree(height=3, scroll=1)

-        netdevs = self.anaconda.network.available()
+        netdevs = self.anaconda.network.netdevices
        devs = netdevs.keys()
        devs.sort()
        ksdevice = self.anaconda.network.getKSDevice()
@@ -178,7 +178,7 @@ class NetworkConfiguratorText:
        #self._ipv6Toggled()
        self._dhcpToggled()

-        netdevs = self.anaconda.network.available()
+        netdevs = self.anaconda.network.netdevices

        while True:
            result = grid.run()
diff --git a/textw/partition_text.py b/textw/partition_text.py
index 193ff21..c9cac6f 100644
--- a/textw/partition_text.py
+++ b/textw/partition_text.py
@@ -160,3 +160,4 @@ class PartitionTypeWindow:

        return INSTALL_OK

+


Unnecessary whitespace change in textw/partition_text.py


Will fix it.


Thanks for the review,

Radek


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]