Am 10. August 2011 00:05 schrieb Andrea Spadaccini <[email protected]>: > --- a/lib/constants.py > +++ b/lib/constants.py > +# Regex used to find IP addresses in the output of ip > +IP_RE = re.compile(r"(inet6?)\s+([.:a-z0-9]+)/", re.IGNORECASE)
I think this constant should go to netutils.py. It's not really of use elsewhere. > +VALID_IP_VERSIONS = frozenset([IP4_VERSION, IP6_VERSION]) Please reformat according to <http://code.google.com/p/ganeti/wiki/StyleGuide#Formatting_sequences>: VALID_IP_VERSIONS = frozenset([ IP4_VERSION, IP6_VERSION, ]) > --- a/lib/netutils.py > +++ b/lib/netutils.py > +def GetInterfaceIpAddresses(ifname): > + """Returns the IP addresses associated to the interface. > + > + @type ifname: string > + @param ifname: Name of the network interface > + @return: A dict having for keys the IP version (either > + L{constants.IP4_VERSION} or L{constants.IP6_VERSION}) and for > + values the lists of IP addresses of the respective version > + associated to the interface > + > + """ > + result = utils.RunCmd([constants.IP_COMMAND_PATH, "-o", "addr", "show", > + ifname]) > + > + if result.failed: > + logging.error("Error running the ip command while getting the IP" > + " addresses of %s", ifname) > + return None > + > + # We use "inet" and "inet6" as keys because this is what we get from the > + # regex. Will later be converted to constants.IP_* > + addr_temp = {"inet": [], "inet6": []} name2ipver = { "inet": constants.IP4_VERSION, "inet6": constants.IP6_VERSION, } result = dict((name, []) for i in name2ipver.values()) > + for row in result.output.split("\n"): Can't you use .splitlines()? > + match = constants.IP_RE.search(row) > + if match: > + addr_temp[match.group(1)].append(match.group(2)) result[name2ipver[match.group(1)]].append(match.group(2)) You should also add verification using inet_pton, i.e. only add the address if it's valid. > + > + addr_dict = {constants.IP4_VERSION: addr_temp["inet"], > + constants.IP6_VERSION: addr_temp["inet6"]} > + > + return addr_dict return result > + > def GetHostname(name=None, family=None): > """Returns a Hostname object. > > @@ -366,7 +447,7 @@ class IPAddress(object): > @type address: str > @param address: ip address whose family will be returned > @rtype: int > - @return: socket.AF_INET or socket.AF_INET6 > + @return: C{socket.AF_INET} or C{socket.AF_INET6} I guess you didn't intend this change here, but rather in one of your functions? > --- a/test/ganeti.constants_unittest.py > +++ b/test/ganeti.constants_unittest.py > @@ -95,6 +95,48 @@ class TestParameterNames(unittest.TestCase): > (kind, key)) > > > +class TestIpRegex(unittest.TestCase): > + """Test the IP address search regular expressions""" > + > + def _checkMatch(self, match, expected_family, expected_addr): > + return (match and match.group(1) == expected_family > + and match.group(2) == expected_addr) > + > + def testIp4(self): > + valid_addresses = [constants.IP4_ADDRESS_ANY, > + constants.IP4_ADDRESS_LOCALHOST, > + "4.4.4.4", > + "192.168.0.1", You should use IP addresses from RFC5737 and RFC3849, see Ganeti's revision history. > + ] > + for addr in valid_addresses: > + fake_ip_output = " inet %s/8 scope host lo" % addr > + > self.failUnless(self._checkMatch(constants.IP_RE.search(fake_ip_output), > + "inet", addr)) If you separate the parsing from actually running the command in netutils.py, you can actually test the parsing and don't have to hope netutils.py uses the regular expression in the same way. Just split the parsing into a “private” function (e.g. “_GetIpAddressFromIpOutput”) and use it here. Michael
