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 <masa...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Igor Lvovsky <ilvov...@redhat.com> Gerrit-Reviewer: Livnat Peer <lp...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches