Dan Kenigsberg has posted comments on this change.
Change subject: BZ#811807 Define network filter on libvirt
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(6 inline comments)
I think we have more simplifications to perform, I hope you do not mind.
....................................................
File vdsm/Makefile.am
Line 52: tc.py \
Line 53: vdsmDebugPlugin.py \
Line 54: vmChannels.py \
Line 55: vm.py \
Line 56: nwfilter.py
please keep sorted. and while at it, would you add
$(NULL)
to the end of the list?
Line 57:
Line 58: dist_vdsmpylib_PYTHON = \
Line 59: __init__.py \
Line 60: define.py \
....................................................
File vdsm/nwfilter.py
Line 34:
Line 35: try:
Line 36: NoMacSpoofingFilter().defineNwFilter(conn)
Line 37: except libvirt.libvirtError, e:
Line 38: traceback.print_exc()
sorry, I still do not understand why you bother to catch an exception only to
log and re-raise it. think positive. exceptions should simplify code, not
complicate it.
Line 39: logging.error("Failed to define network filter: %s" %
e.message)
Line 40: raise
Line 41: finally:
Line 42: try:
Line 39: logging.error("Failed to define network filter: %s" %
e.message)
Line 40: raise
Line 41: finally:
Line 42: try:
Line 43: conn.close()
I know that some people do not trust the garbage collector to close such
objects. fine. I understand that. but why worry about the exception?
Line 44: except libvirt.libvirtError:
Line 45: pass
Line 46:
Line 47: class NwFilter(object):
Line 71: # is being used by running VMs
Line 72: pass
Line 73:
Line 74: try:
Line 75: nwFilter = conn.nwfilterDefineXML(self.buildFilterXml())
again, you assign a value to a variable that no one care about.
Line 76: except libvirt.libvirtError:
Line 77: logging.error("Failed to define filter %s" %
(self.filterName))
Line 78: raise
Line 79: else:
Line 73:
Line 74: try:
Line 75: nwFilter = conn.nwfilterDefineXML(self.buildFilterXml())
Line 76: except libvirt.libvirtError:
Line 77: logging.error("Failed to define filter %s" %
(self.filterName))
again, I see no point in this explicit log. the default traceback produced by
unhandled exception is good enough.
Line 78: raise
Line 79: else:
Line 80: logging.debug("Filter %s was defined" % (nwFilter.name()))
Line 81:
Line 90: NwFilter.__init__(self, 'vdsm-no-mac-spoofing')
Line 91:
Line 92: def getFilterXml(self):
Line 93: return '''<filter name='%s' chain='root'>
Line 94: <!-- preventing MAC spoofing -->
I this kind of programmer that actually needs comments. However, I find these
two as adding very very little over the finely-named filters.
Line 95: <filterref filter='no-mac-spoofing'/>
Line 96: <!-- preventing ARP MAC spoofing -->
Line 97: <filterref filter='no-arp-mac-spoofing'/>
Line 98: </filter> '''
--
To view, visit http://gerrit.ovirt.org/7354
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f1708385dec6a87bc404e4ab25c4da8ab8a8acc
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Moti Asayag <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Igor Lvovsky <[email protected]>
Gerrit-Reviewer: Livnat Peer <[email protected]>
Gerrit-Reviewer: Moti Asayag <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
