Change in vdsm[master]: netinfo: assess DHCP on devices from cmdlines of dhclients

osvoboda at redhat.com osvoboda at redhat.com
Tue Dec 8 07:56:44 UTC 2015


Ondřej Svoboda has posted comments on this change.

Change subject: netinfo: assess DHCP on devices from cmdlines of dhclients
......................................................................


Patch Set 11:

(4 comments)

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
Fair enough :-)


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
Of course, it is api.setupNetworks. Right now, I cannot see anymore how its

     libvirt_nets = netinfo_networks()
    _netinfo = NetInfo(_netinfo=netinfo_get(libvirtNets2vdsm(libvirt_nets)))

is different from just

    _netinfo = NetInfo()


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?
I wanted to have a neat container to replace the usage of two separate sets (or a tuple with them) and to access the sets with nice syntax.

Regardless of the above, the class doesn't survive the basic question on its purpose.


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 th
I cannot see what I could actually do now.

Run several dhclients with cmdline arguments (claimed to be) understood by the new routine and see if they are detected?

Functional tests do this for me indirectly :-)


-- 
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 <osvoboda at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh at redhat.com>
Gerrit-Reviewer: Ido Barkan <ibarkan at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Ondřej Svoboda <osvoboda at redhat.com>
Gerrit-Reviewer: Petr Horáček <phoracek at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list