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 <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
