Ido Barkan has posted comments on this change. Change subject: netinfo: assess DHCP on devices from cmdlines of dhclients ......................................................................
Patch Set 11: (5 comments) Petr, Can you review the ovs hooks change part? https://gerrit.ovirt.org/#/c/49097/11/lib/vdsm/netinfo/__init__.py File lib/vdsm/netinfo/__init__.py: Line 36: from .dhcp import get_devices_with_dhcp I really tried to use relative naming from now on. in the old god module of netinfo we needed a name that indicates this function is about dhcp. but now we just import dhcp and call: dhcp.get_devices() this is not implemented here yet, but is fixed soon in patches like https://gerrit.ovirt.org/#/c/49624/ Line 91: if routes is None: : routes = get_routes() : if ipAddrs is None: : ipAddrs = getIpAddrs() : if devices_with_dhcp is None: : devices_with_dhcp = get_devices_with_dhcp() I hate making this function semi-automatic (although you are just repeating this existing horror). Can you find out if anyone calls libvirtNets2vdsm without devices_with_dhcp? https://gerrit.ovirt.org/#/c/49097/11/lib/vdsm/netinfo/dhcp.py File lib/vdsm/netinfo/dhcp.py: Line 27: DhcpDevices: why do you need a class here? can't you manage a dict with 2 keys? {'dhcpv4': set(), 'dhcpv6': set()} Line 56: argv can this really be an empty tuple? getCmdArgs can do this (safely) but do you want to ignore parse_cmdline empty result? https://gerrit.ovirt.org/#/c/49097/11/tests/netinfoTests.py File tests/netinfoTests.py: Line 238: why not converting this test to test pgrep way? If you previously needed this unit test in addition to functional tests why are you happy now without one? -- To view, visit https://gerrit.ovirt.org/49097 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I160a0e2d4c734de94b0995e7433216baa88972ab Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Ondřej Svoboda <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Edward Haas <[email protected]> Gerrit-Reviewer: Ido Barkan <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Ondřej Svoboda <[email protected]> Gerrit-Reviewer: Petr Horáček <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
