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