[PATCH phosphor-networkd v2] Adding org.openbmc.NetworkManager dbus interface for network configuation.
Chris Austen
austenc at us.ibm.com
Wed Feb 3 13:48:26 AEDT 2016
Chris Austen
POWER Systems Enablement Manager
(512) 286-5184 (T/L: 363-5184)
-----"openbmc" <openbmc-bounces+austenc=us.ibm.com at lists.ozlabs.org> wrote: -----
To: OpenBMC Patches <openbmc-patches at stwcx.xyz>, hramasub at in.ibm.com
From: Cyril Bur
Sent by: "openbmc"
Date: 02/02/2016 07:01PM
Cc: openbmc at lists.ozlabs.org
Subject: Re: [PATCH phosphor-networkd v2] Adding org.openbmc.NetworkManager dbus interface for network configuation.
On Mon, 1 Feb 2016 16:30:34 -0600
OpenBMC Patches <openbmc-patches at stwcx.xyz> wrote:
> From: Hariharasubramanian R <hramasub at in.ibm.com>
>
After all the previous discussion about Python indenting, I thought we were
sticking to spaces. I've outlined some tabs I noticed.
I assume this python will one day be rewritten in C?
> ---
> netman.py | 147 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 147 insertions(+)
> create mode 100755 netman.py
>
> diff --git a/netman.py b/netman.py
> new file mode 100755
> index 0000000..c63921f
> --- /dev/null
> +++ b/netman.py
> @@ -0,0 +1,147 @@
> +#!/usr/bin/env python
> +
> +from subprocess import call
> +import sys
> +import subprocess
> +import dbus
> +import string
> +import os
> +import fcntl
> +import glib
> +import gobject
> +import dbus.service
> +import dbus.mainloop.glib
> +
> +DBUS_NAME = 'org.openbmc.NetworkManager'
> +OBJ_NAME = '/org/openbmc/NetworkManager/Interface'
> +
> +network_providers = {
> + 'networkd' : {
> + 'bus_name' : 'org.freedesktop.network1',
> + 'ip_object_name' : '/org/freedesktop/network1/network/default',
> + 'hw_object_name' : '/org/freedesktop/network1/link/_31',
> + 'interface_name' : 'org.freedesktop.network1.Network',
> + 'method' : 'org.freedesktop.network1.Network.SetAddr'
> + },
> + 'NetworkManager' : {
> + 'bus_name' : 'org.freedesktop.NetworkManager',
> + 'ip_object_name' : '/org/freedesktop/NetworkManager',
> + 'hw_object_name' : '/org/freedesktop/NetworkManager',
> + 'interface_name' : 'org.freedesktop.NetworkManager',
> + 'method' : 'org.freedesktop.NetworkManager' # FIXME:
> + },
> +}
Have I misunderstood PEP8, isn't it always spaces for indenting?
> +
> +def getPrefixLen(mask):
> + prefixLen = sum([bin(int(x)).count('1') for x in mask.split('.')])
> + return prefixLen
Tabs here... ^
> +
> +class IfAddr ():
> + def __init__ (self, family, scope, flags, prefixlen, addr, gw):
> + self.family = family
> + self.scope = scope
> + self.flags = flags
> + self.prefixlen = prefixlen
> + self.addr = addr
> + self.gw = gw
PEP8 discourages extraneous whitespace like this...
> +
> +class NetMan (dbus.service.Object):
> + def __init__(self, bus, name):
> + self.bus = bus
> + self.name = name
> + dbus.service.Object.__init__(self,bus,name)
> +
> + def setNetworkProvider(self, provider):
> + self.provider = provider
> +
> + def _setAddr (self, op, device, ipaddr, netmask, family, flags, scope, gateway):
> + netprov = network_providers [self.provider]
> + bus_name = netprov ['bus_name']
> + obj_path = netprov ['ip_object_name']
> + intf_name = netprov ['interface_name']
PEP8 discourages extraneous whitespace...
> +
> + obj = self.bus.get_object(bus_name, obj_path)
> + intf = dbus.Interface(obj, intf_name)
> + if (op == "add"):
> + return intf.AddAddress (device, ipaddr, netmask, family, flags, scope, gateway)
> +
> + if (op == "del"):
> + return intf.DelAddress (device, ipaddr, netmask, family, flags, scope, gateway)
> +
> + def _getAddr (self, target, device):
> + netprov = network_providers [self.provider]
> + bus_name = netprov ['bus_name']
> +
> + if (target == "ip"):
> + intf_name = 'org.freedesktop.network1.Network'
> + obj_path = netprov ['ip_object_name']
> + obj = self.bus.get_object(bus_name, obj_path)
> + intf = dbus.Interface(obj, intf_name)
> + return intf.GetAddress (device)
> +
> + if (target == "mac"):
> + intf_name = 'org.freedesktop.network1.Link'
> + obj_path = netprov ['hw_object_name']
> + obj = self.bus.get_object(bus_name, obj_path)
> + intf = dbus.Interface(obj, intf_name)
> + mac = intf.GetAddress (device)
> + print mac
> + return mac
> +
> +
> +
> + @dbus.service.method(DBUS_NAME, "", "")
> + def test(self):
> + print("TEST")
Should this code stay in the final version?
> +
> + @dbus.service.method(DBUS_NAME, "ssss", "x")
> + def AddAddress4 (self, device, ipaddr, netmask, gateway):
> + prefixLen = getPrefixLen (netmask)
> + confFile = "/etc/systemd/network/10-bmc-" + device + "-" + ipaddr + '_' + str(prefixLen) + ".network"
if you put the ip in the name of the file then you can create multiple .networks. How is the networkd program suppose to know which one to run?...
10-bmc-eth0-192.168.1.2.network
10-bmc-eth0-192.168.1.3.network
10-bmc-eth0-10.10.0.2.network
I would recommend hardcoding the name and just overwrite it with the new values like you did for the EnableDHCP write.
You should build the confFile in both the AddAddress4 and EnableDHCP from one function. That way you never get out of sync or forget to change a naming convention in two different places in the code.
> + if os.path.exists(confFile):
> + return 0
Delete this check once you change the naming convention of the .network
> +
> + print("Making .network file...")
> + networkconf = open (confFile, "w+")
> + networkconf.write ('[Match]'+ '\n')
> + networkconf.write ('Name=' + (device) + '\n')
> + networkconf.write ('[Network]' + '\n')
> + networkconf.write ('Address=' + ipaddr + '/' + str(prefixLen) + '\n')
> + networkconf.write ('Gateway=' + gateway + '\n')
> +
> + print("Restarting networkd service...")
> + call(["systemctl", "restart", "systemd-networkd.service"])
This line should be call(["ip", "addr", "flush", device])
> + return 0
> + #return self._setAddr ("add", device, ipaddr, netmask, 2, 0, 253, gateway)
Why is that return statement commented?
> +
> + @dbus.service.method(DBUS_NAME, "ssss", "x")
> + def DelAddress4 (self, device, ipaddr, netmask, gateway):
> + prefixLen = getPrefixLen (netmask)
> + confFile = "/etc/systemd/network/10-bmc-" + device + "-" + ipaddr + '_' + str(prefixLen) + ".network"
> + if not (os.path.exists(confFile)):
> + return 1
> +
> + self._setAddr ("del", device, ipaddr, netmask, 2, 0, 253, gateway)
> + os.remove (confFile)
> + return 0;
> +
CA: does it make sense to require the ip, netmask and gateway to delete an interface? Shouldn't it just be "eth0" ? It's not as if eth0 can be bounded to multiple ipv4 addresses. It appears that you needed the extra strings because you needed to build the name of the file. I think my other suggestions would solve this issue.
> + @dbus.service.method(DBUS_NAME, "s", "a(iyyus)s")
> + def GetAddress4 (self, device):
> + return self._getAddr ("ip", device)
> +
> + @dbus.service.method(DBUS_NAME, "s", "s")
> + def GetHwAddress (self, device):
> + return self._getAddr ("mac", device)
> +
> +def main():
> + dbus.mainloop.glib.DBusGMainLoop(set_as_default=True)
> + bus = dbus.SystemBus()
> + name = dbus.service.BusName(DBUS_NAME, bus)
> + obj = NetMan (bus, OBJ_NAME)
> + obj.setNetworkProvider ("networkd")
> + mainloop = gobject.MainLoop()
> + print("Started")
> + mainloop.run()
> +
> +if __name__ == '__main__':
> + sys.exit(main())
_______________________________________________
openbmc mailing list
openbmc at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/openbmc
More information about the openbmc
mailing list