Dan Kenigsberg has posted comments on this change.
Change subject: hooks: macbind - Attach vm nics to specified bridge
......................................................................
Patch Set 7: Code-Review-1
(5 comments)
http://gerrit.ovirt.org/#/c/17895/7/vdsm.spec.in
File vdsm.spec.in:
Line 396: without the sharing and storage handling of VDSM.
Line 397:
Line 398: %package hook-macbind
Line 399: Summary: Bind a vNIC to a Bridge
Line 400: BuildArch: noarch
please requires a specific version of vdsm, as you are using an internal API thereof
(ipwtapper.Link)
Line 401:
Line 402: %description hook-macbind
Line 403: VDSM hooks which allow to bind a vNIC to a Bridge, managed or not by engine.
Line 404:
http://gerrit.ovirt.org/#/c/17895/7/vdsm_hooks/macbind/before_vm_start.py
File vdsm_hooks/macbind/before_vm_start.py:
Line 38: macconfig = ast.literal_eval(os.environ['macbind'])
Line 39: macconfig = dict((k.lower(), v)
Line 40: for k, v in macconfig.iteritems())
Line 41: domxml = hooking.read_domxml()
Line 42: for devicemac in macconfig:
please move this block (that has macconfig and domxml ready) to a helper function that can
be tested properly. Let's call it "replaceSourceBridge(macconfig, domxml,
devType)"
Line 43: if isDevice(macconfig[devicemac]):
Line 44: devType = Link._detectType(macconfig[devicemac])
Line 45: if devType in _DEV_TYPE:
Line 46: for iface in
domxml.getElementsByTagName('interface'):
Line 59: domxml,
Line 60: 'virtualport',
Line 61: 'type',
Line 62: 'openvswitch'))
Line 63: else:
do you intentionally want to continue starting the VM, despite the mismatching custom
property? Why? I think it would be safe to raise an exception here.
Line 64: sys.stderr.write(" Invalid device type [%s]" %
Line 65: devType)
Line 66: else:
Line 67: sys.stderr.write(" Invalid iface [%s]" %
Line 68: macconfig[devicemac])
Line 69: hooking.write_domxml(domxml)
Line 70:
Line 71: except:
Line 72: hooking.exit_hook('macbind: [unexpected error]: %s\n'
I see no need to handle exception within the main() function. The handling in line 106 is
enough.
Line 73: % traceback.format_exc())
Line 74: sys.exit(2)
Line 75:
Line 76:
Line 73: % traceback.format_exc())
Line 74: sys.exit(2)
Line 75:
Line 76:
Line 77: def test():
what does this function test? It certainly does not test main() (which it does not
call)...
It should instead call replaceSourceBridge(), so we can see that it handles xml properly.
Line 78:
Line 79: text = '''<interface type="bridge">
Line 80: <address bus="0x00" domain="0x0000"
function="0x0" slot="0x03" type="pci"/>
Line 81: <mac address="00:1a:4a:60:d1:9a"/>
--
To view, visit
http://gerrit.ovirt.org/17895
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0356dfab224a9082b44aae1c66df050f7456301c
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Humble Devassy Chirammal <hchiramm(a)redhat.com>
Gerrit-Reviewer: Amador Pahim <apahim(a)redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap(a)redhat.com>
Gerrit-Reviewer: Aravinda VK <avishwan(a)redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland(a)redhat.com>
Gerrit-Reviewer: Giuseppe Vallarelli <gvallare(a)redhat.com>
Gerrit-Reviewer: Humble Devassy Chirammal <hchiramm(a)redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw(a)linux.vnet.ibm.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes