Change in vdsm[master]: tc: Integrate Host QoS with network deletion via setupNetworks

danken at redhat.com danken at redhat.com
Tue Sep 9 23:52:21 UTC 2014


Dan Kenigsberg has posted comments on this change.

Change subject: tc: Integrate Host QoS with network deletion via setupNetworks
......................................................................


Patch Set 6:

(4 comments)

http://gerrit.ovirt.org/#/c/31440/6/vdsm/network/configurators/qos.py
File vdsm/network/configurators/qos.py:

Line 60:     except tc.TrafficControlException as tce:
Line 61:         if tce.errCode != errno.EINVAL:  # no filter exists
Line 62:             raise
Line 63: 
Line 64:     qdiscs = list(tc._qdiscs(device))
Calling list() is redundant; I'm not judging the intermediate variable.
Line 65:     root_qdisc_handle = _root_qdisc(qdiscs)['handle']
Line 66:     try:
Line 67:         tc.cls.delete(device, classid=root_qdisc_handle + class_id)
Line 68:     except tc.TrafficControlException as tce:


Line 70:             raise
Line 71: 
Line 72:     if _no_used_classes(device, parent=root_qdisc_handle):
Line 73:         tc._qdisc_del(device)
Line 74:         tc._qdisc_del(device, kind='ingress')
pardon my lapse, but where did we add an ingress qdisc?
Line 75: 
Line 76: 
Line 77: def _no_used_classes(device, parent):
Line 78:     """Returns true iff there's no traffic classes in the device, ignoring the


Line 73:         tc._qdisc_del(device)
Line 74:         tc._qdisc_del(device, kind='ingress')
Line 75: 
Line 76: 
Line 77: def _no_used_classes(device, parent):
negation in function names tends to confuse me. Can you find a positive name? Maybe _uses_classes()?

The "parent" arg is not well defined, and probably not independent of "device"... "parent" has to be root_qdisc_handle of the "device", isn't it? it's passed as an arg only as a short cut? This should be documented, or expressed by another arg name.
Line 78:     """Returns true iff there's no traffic classes in the device, ignoring the
Line 79:     root class and a default unused class"""
Line 80:     classes = list(tc._classes(device, parent=parent))
Line 81:     classes = [cls for cls in classes if not cls.get('root')]


Line 76: 
Line 77: def _no_used_classes(device, parent):
Line 78:     """Returns true iff there's no traffic classes in the device, ignoring the
Line 79:     root class and a default unused class"""
Line 80:     classes = list(tc._classes(device, parent=parent))
Calling list() is redundant
Line 81:     classes = [cls for cls in classes if not cls.get('root')]
Line 82:     return (not classes or
Line 83:             (len(classes) == 1 and not netinfo.ifaceUsed(device) and
Line 84:              classes[0]['handle'] == parent + _DEFAULT_CLASSID))


-- 
To view, visit http://gerrit.ovirt.org/31440
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I28a3676af1553052e7cdea515faa29e847b7d3c0
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list