-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On Tue, 4 May 2010, Radek Vykydal wrote:
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.
It is probably fine then, and consistency is a good thing. Since upgrade and
rescue modes both rely on what loader sets up, we are still getting ifcfg
files to use, so what you are doing here is good. No problems from me.
- 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).
See it now, looks good.
- 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.
You are correct. The networking stack has changed so much in anaconda that
this is leftover code from things we used to do. No problems from me.
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.
I think that would be a nice improvement, but it can wait for another patch.
- 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,
- --
David Cantrell <dcantrell@xxxxxxxxxx>
Red Hat / Honolulu, HI
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
iEYEARECAAYFAkvi2KEACgkQ5hsjjIy1Vkmh2QCgnpQQPcCCwkIoWirwjOK4+FcP
1mwAniWoT3hvkdMIArnhRna+PvTG4tKc
=VN4I
-----END PGP SIGNATURE-----
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
