All comments addressed. Please glance over the changes if you get a chance.
Diff comments:
> diff --git a/cloudinit/netinfo.py b/cloudinit/netinfo.py
> index 993b26c..081f7b4 100644
> --- a/cloudinit/netinfo.py
> +++ b/cloudinit/netinfo.py
> @@ -18,18 +21,81 @@ from cloudinit.simpletable import SimpleTable
> LOG = logging.getLogger()
>
>
> -def netdev_info(empty=""):
> - fields = ("hwaddr", "addr", "bcast", "mask")
> - (ifcfg_out, _err) = util.subp(["ifconfig", "-a"], rcs=[0, 1])
> +DEFAULT_NETDEV_INFO = {
> + "addr": "",
> + "bcast": "",
> + "hwaddr": "",
> + "mask": "",
> + "scope": "",
> + "up": False
> +}
> +
> +
> +def netdev_cidr_to_mask(cidr):
> + mask = socket.inet_ntoa(
> + struct.pack(">I", (0xffffffff << (32 - int(cidr)) & 0xffffffff)))
> + return mask
> +
> +
> +def netdev_info_iproute(ipaddr_data, iplink_data):
> + """
> + Get network device dicts from ip route and ip link info.
> +
> + @param ipaddr_data: Output string from ip address command.
> + @param iplink_data: Output string from ip link command.
> +
> + @returns: A dict of device info keyed by network device name containing
> + device configuration values.
> + """
> devs = {}
We won't blowup on empty data, we return devs = {} in that case, which feels
appropriate if no routes are listed in output.
> - for line in str(ifcfg_out).splitlines():
> + for line in str(ipaddr_data).splitlines():
Yeah, I should've looked more at some of this code when I took over the branch.
The util.subp function should take care of this data decoding for us if
necessary, I'd expect it to be handling in a common layer instead of making all
call-sites type-check and decode. This str might have been inherited from when
subp didn't return the right string type.
> + details = line.lower().strip().split()
> + curdev = details[1]
> + fieldpost = ""
Good though, changed to ip_version_suffix and added a comment about its use.
> + if curdev not in devs:
> + devs[curdev] = copy(DEFAULT_NETDEV_INFO)
> + for i in range(len(details)):
> + if details[i] == 'inet':
> + (addr, cidr) = details[i + 1].split("/")
I used partition('/') and set the cidr to '24' if not present. We can look at
consolidating in a separate branch.
> + devs[curdev]["mask"] = netdev_cidr_to_mask(cidr)
Moved it. good thought. I missed the duplication.
> + devs[curdev]["addr"] = addr
> + fieldpost = ""
> + elif details[i] == 'inet6':
> + addr = details[i + 1]
> + devs[curdev]["addr6"] = addr
> + fieldpost = "6"
> + elif details[i] == "scope":
> + devs[curdev]["scope" + fieldpost] = details[i + 1]
> + elif details[i] == "brd":
> + devs[curdev]["bcast" + fieldpost] = details[i + 1]
Took both of your comments. I simplified netdev_info_iproute to parse all of
'ip addr show' in one go. I also tweaked the logic to use regex matches instead
of walking through individual tokens. It's only about the same performance as
far timeit is concerned, bit the regex gives us a better view of the expected
format of the output.
> +
> + for line in str(iplink_data).splitlines():
dropped the str throughout.
> + details = line.lower().strip().split()
> + # Strip trailing ':' and truncate any interface alias '@if34'
> + curdev = details[1].rstrip(':').split('@')[0]
> + for i in range(len(details)):
> + if details[i] == curdev + ":":
> + flags = details[i + 1].strip("<>").split(",")
> + if "lower_up" in flags and "up" in flags:
> + devs[curdev]["up"] = True
> + elif details[i] == "link/ether":
> + devs[curdev]["hwaddr"] = details[i + 1]
> + return devs
> +
> +
> +def netdev_info_ifconfig(ifconfig_data):
> + # fields that need to be returned in devs for each dev
> + devs = {}
> + for line in str(ifconfig_data).splitlines():
dropped the str throughout.
> if len(line) == 0:
> continue
> if line[0] not in ("\t", " "):
> curdev = line.split()[0]
> - devs[curdev] = {"up": False}
> - for field in fields:
> - devs[curdev][field] = ""
> + # current ifconfig pops a ':' on the end of the device
> + if curdev.endswith(':'):
> + curdev = curdev[:-1]
> + if curdev not in devs:
> + devs[curdev] = copy(DEFAULT_NETDEV_INFO)
> toks = line.lower().strip().split()
> if toks[0] == "up":
> devs[curdev]['up'] = True
> @@ -39,41 +105,50 @@ def netdev_info(empty=""):
> if re.search(r"flags=\d+<up,", toks[1]):
> devs[curdev]['up'] = True
>
> - fieldpost = ""
> - if toks[0] == "inet6":
> - fieldpost = "6"
> -
> for i in range(len(toks)):
> - # older net-tools (ubuntu) show 'inet addr:xx.yy',
> - # newer (freebsd and fedora) show 'inet xx.yy'
> - # just skip this 'inet' entry. (LP: #1285185)
> - try:
> - if ((toks[i] in ("inet", "inet6") and
> - toks[i + 1].startswith("addr:"))):
> - continue
> - except IndexError:
> - pass
> -
> - # Couple the different items we're interested in with the correct
> - # field since FreeBSD/CentOS/Fedora differ in the output.
> - ifconfigfields = {
> - "addr:": "addr", "inet": "addr",
> - "bcast:": "bcast", "broadcast": "bcast",
> - "mask:": "mask", "netmask": "mask",
> - "hwaddr": "hwaddr", "ether": "hwaddr",
> - "scope": "scope",
> - }
> - for origfield, field in ifconfigfields.items():
> - target = "%s%s" % (field, fieldpost)
> - if devs[curdev].get(target, ""):
> - continue
> - if toks[i] == "%s" % origfield:
> - try:
> - devs[curdev][target] = toks[i + 1]
> - except IndexError:
> - pass
> - elif toks[i].startswith("%s" % origfield):
> - devs[curdev][target] = toks[i][len(field) + 1:]
> + if toks[i] == "inet":
> + if toks[i + 1].startswith("addr:"):
> + devs[curdev]['addr'] = toks[i + 1].lstrip("addr:")
> + else:
> + devs[curdev]['addr'] = toks[i + 1]
> + elif toks[i].startswith("bcast:"):
> + devs[curdev]['bcast'] = toks[i].lstrip("bcast:")
> + elif toks[i] == "broadcast":
> + devs[curdev]['bcast'] = toks[i + 1]
> + elif toks[i].startswith("mask:"):
> + devs[curdev]['mask'] = toks[i].lstrip("mask:")
> + elif toks[i] == "netmask":
> + devs[curdev]['mask'] = toks[i + 1]
> + elif toks[i] == "hwaddr" or toks[i] == "ether":
> + devs[curdev]['hwaddr'] = toks[i + 1]
> + elif toks[i] == "inet6":
> + if toks[i + 1] == "addr:":
> + devs[curdev]['addr6'] = toks[i + 2]
> + else:
> + devs[curdev]['addr6'] = toks[i + 1]
> + elif toks[i] == "prefixlen":
> + addr6 = devs[curdev]['addr6'] + "/" + toks[i + 1]
> + devs[curdev]['addr6'] = addr6
> + elif toks[i].startswith("scope:"):
> + devs[curdev]['scope6'] = toks[i].lstrip("scope:")
> + elif toks[i] == "scopeid":
> + res = re.match(".*<(\S+)>", toks[i + 1])
> + if res:
> + devs[curdev]['scope6'] = res.group(1)
> + return devs
> +
> +
> +def netdev_info(empty=""):
> + devs = {}
> + try:
> + # Try iproute first of all
> + (ipaddr_out, _err) = util.subp(["ip", "--oneline", "addr", "list"])
> + (iplink_out, _err) = util.subp(["ip", "--oneline", "link", "list"])
> + devs = netdev_info_iproute(ipaddr_out, iplink_out)
> + except util.ProcessExecutionError:
> + # Fall back to net-tools if iproute2 is not present
> + (ifcfg_out, _err) = util.subp(["ifconfig", "-a"], rcs=[0, 1])
> + devs = netdev_info_ifconfig(ifcfg_out)
+1 great point. fixed.
>
> if empty != "":
> for (_devname, dev) in devs.items():
> @@ -125,31 +279,53 @@ def route_info():
> routes['ipv4'].append(entry)
>
> try:
> - (route_out6, _err6) = util.subp(["netstat", "-A", "inet6", "-n"],
> - rcs=[0, 1])
> + (route_data6, _err6) = util.subp(
> + ["netstat", "-A", "inet6", "--route", "--numeric"], rcs=[0, 1])
> except util.ProcessExecutionError:
> pass
> else:
> - entries6 = route_out6.splitlines()[1:]
> + entries6 = route_data6.splitlines()
> for line in entries6:
> if not line:
> continue
> toks = line.split()
> - if (len(toks) < 6 or toks[0] == "Kernel" or
> + if (len(toks) < 7 or toks[0] == "Kernel" or
> + toks[0] == "Destination" or toks[0] == "Internet" or
> toks[0] == "Proto" or toks[0] == "Active"):
> continue
> entry = {
> - 'proto': toks[0],
> - 'recv-q': toks[1],
> - 'send-q': toks[2],
> - 'local address': toks[3],
> - 'foreign address': toks[4],
> - 'state': toks[5],
> + 'destination': toks[0],
> + 'gateway': toks[1],
> + 'flags': toks[2],
> + 'metric': toks[3],
> + 'ref': toks[4],
> + 'use': toks[5],
> + 'iface': toks[6],
> }
> + # skip lo interface on ipv6
> + if entry['iface'] == "lo":
> + continue
> + # strip /128 from address if it's included
> + if entry['destination'].endswith('/128'):
> + entry['destination'] = entry['destination'][:-4]
Good suggestion, though I went with re.sub(r'\/128$', '', ...) here just to be
explicit.
> routes['ipv6'].append(entry)
> return routes
>
>
> +def route_info():
> + routes = {}
> + try:
> + # Try iproute first of all
> + (iproute_out, _err) = util.subp(["ip", "-o", "route", "list"])
> + routes = netdev_route_info_iproute(iproute_out)
> + except util.ProcessExecutionError:
> + # Fall back to net-tools if iproute2 is not present
> + (route_out, _err) = util.subp(
> + ["netstat", "--route", "--numeric", "--extend"], rcs=[0, 1])
> + routes = netdev_route_info_netstat(route_out)
> + return routes
> +
> +
> def getgateway():
> try:
> routes = route_info()
--
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/342428
Your team cloud-init commiters is requested to review the proposed merge of
~chad.smith/cloud-init:net-tools-deprecation-plus-review-comments into
cloud-init:master.
_______________________________________________
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help : https://help.launchpad.net/ListHelp
