Change in vdsm[master]: BZ#811807 Define network filter on libvirt

danken at redhat.com danken at redhat.com
Tue Aug 21 02:54:45 UTC 2012


Dan Kenigsberg has posted comments on this change.

Change subject: BZ#811807 Define network filter on libvirt
......................................................................


Patch Set 2: I would prefer that you didn't submit this

(9 inline comments)

....................................................
File vdsm/nwfilter.py
Line 1: #! /usr/bin/python
Line 2: # Copyright 2012 IBM, Inc.
should I have heard about the changes in your affiliation?
Line 3: #
Line 4: # This program is free software; you can redistribute it and/or modify
Line 5: # it under the terms of the GNU General Public License as published by
Line 6: # the Free Software Foundation; either version 2 of the License, or


Line 18: # Refer to the README and COPYING files for full details of the license
Line 19: #
Line 20: 
Line 21: import libvirt
Line 22: import logging
start with standard lib import

then site-packages (such as libvirt)

then internal project.
Line 23: from vdsm import libvirtconnection
Line 24: 
Line 25: 
Line 26: def main():


Line 30:     conn = libvirtconnection.get()
Line 31:     defineNwFilter(conn, NoMacSpoofingFilter())
Line 32:     try:
Line 33:         conn.close()
Line 34:     except:
catch-all-exception is generally evil. why is it needed now?
Line 35:         pass
Line 36: 
Line 37: def defineNwFilter(conn, nwFilter):
Line 38:     """


Line 39:     define vdsm network filter on libvirt to control VM traffic
Line 40:     """
Line 41: 
Line 42:     filter = None
Line 43:     filterName = nwFilter.filterName
seems like defineNwFilter could have been a method of VdsmNwFilter
Line 44:     filterXml = nwFilter.buildFilterXml()
Line 45: 
Line 46:     try:
Line 47:         filter = conn.nwfilterLookupByName(filterName)


Line 43:     filterName = nwFilter.filterName
Line 44:     filterXml = nwFilter.buildFilterXml()
Line 45: 
Line 46:     try:
Line 47:         filter = conn.nwfilterLookupByName(filterName)
it would be much cleaner to have 

 filter = conn.nwfilterLookupByName(filterName)
 filter.undefine()

in one blow. or even 

 conn.nwfilterLookupByName(filterName).undefine()

as no one cares for the "filter variable"
Line 48:     except libvirt.libvirtError:
Line 49:         logging.debug("No such filter: %s" % (filterName))
Line 50: 
Line 51:     if filter:


Line 56:             # Ignore failure if filter already exists. Failure might occur when
Line 57:             # attempting to remove a filter which is being used by running VMs
Line 58:             pass
Line 59:     try:
Line 60:         nwFilter = conn.nwfilterDefineXML(filterXml)
no need to set a variable that is never used. Doesn't pyflake cry about this?
Line 61:     except libvirt.libvirtError:
Line 62:         logging.error("Failed to define filter %s" % (filterName))
Line 63:         raise
Line 64:     else:


Line 59:     try:
Line 60:         nwFilter = conn.nwfilterDefineXML(filterXml)
Line 61:     except libvirt.libvirtError:
Line 62:         logging.error("Failed to define filter %s" % (filterName))
Line 63:         raise
exceptions should be caught at the top-most level possible. you can log the whole traceback up there.
Line 64:     else:
Line 65:         logging.debug("Filter %s was defined" % (nwFilter.name()))
Line 66: 
Line 67: 


Line 64:     else:
Line 65:         logging.debug("Filter %s was defined" % (nwFilter.name()))
Line 66: 
Line 67: 
Line 68: class VdsmNwFilter(object):
we all know this is Vdsm here. No need for the Vdsm prefix.
Line 69:     """
Line 70:     Base class for custom network filters
Line 71:     """
Line 72: 


....................................................
File vdsm/vdsmd.init.in
Line 482:        log_failure_msg "$prog: one of the dependent services did not start, error code $ret_val"
Line 483:        return $ret_val
Line 484:     fi
Line 485: 
Line 486:     @VDSMDIR@/nwfilter.py > /dev/null 2>&1
do we really want to hide stderr? in the rare case of an error, I'd like to see its nature.
Line 487:     ret_val=$?
Line 488:     if [ $ret_val -ne 0 ]
Line 489:     then
Line 490:         log_failure_msg "$prog: Failed to define network filters on libvirt"


--
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: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Moti Asayag <masayag at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky at redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer at redhat.com>
Gerrit-Reviewer: Moti Asayag <masayag at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list