Antoni Segura Puimedon has posted comments on this change. Change subject: tc: replace mirred filter parsing with a generic filter parser ......................................................................
Patch Set 6: (10 comments) http://gerrit.ovirt.org/#/c/29211/6/vdsm/network/tc/__init__.py File vdsm/network/tc/__init__.py: Line 177: Filter = namedtuple('Filter', 'prio handle actions') Line 178: MirredAction = namedtuple('MirredAction', 'target') Line 179: Line 180: Line 181: def _filter_show(dev, parent=None, prio=None): > is this needed? it looks just like the new tc.filter.show() function Good catch. It is dead code now. Line 182: cmd = ['filter', 'show', 'dev', dev] Line 183: if parent is not None: Line 184: cmd += ['parent', parent] Line 185: if prio is not None: Line 215: next(tokens) # Consume kind (qdisc, class, filter, etc) Line 216: yield module.parse(tokens) Line 217: Line 218: Line 219: _filters = partial(_iterate, tc_filter) # Useful to pass parent > I do not understand the comment Done http://gerrit.ovirt.org/#/c/29211/6/vdsm/network/tc/_parser.py File vdsm/network/tc/_parser.py: Line 28: def linearize(inp): Line 29: """Generator of tc entries (that can span over multiple textual lines""" Line 30: current = [] Line 31: for line in inp: Line 32: line = line.replace('\t', ' ') > this replace() can be avoided by adding Done Line 33: if line.startswith(' '): Line 34: current.append(0) Line 35: current.extend(line.strip().split()) Line 36: else: Line 30: current = [] Line 31: for line in inp: Line 32: line = line.replace('\t', ' ') Line 33: if line.startswith(' '): Line 34: current.append(0) > this is the very same '\0' of line 24 above, right? A constant would make Done Line 35: current.extend(line.strip().split()) Line 36: else: Line 37: if current: Line 38: yield current Line 40: if current: Line 41: yield current Line 42: Line 43: Line 44: def tokenize(inp): > this "inp" seems much different than linearize()'s. Please define the time Done Line 45: """Generator of tokens of a textual input""" Line 46: for token in inp: http://gerrit.ovirt.org/#/c/29211/6/vdsm/network/tc/filter.py File vdsm/network/tc/filter.py: Line 28: command += ['pref', pref] Line 29: return _wrapper.process_request(command) Line 30: Line 31: Line 32: def parse(tokens): > A function-level unit test would make it easier for me (or any other newcom added test of tc._filters Line 33: """Parses a filter entry token generator into a data dictionary""" Line 34: data = {} Line 35: for token in tokens: Line 36: if token == 'root': Line 57: elif token in ('*flowid', 'flowid'): Line 58: data['flowid'] = next(tokens) Line 59: elif token == 'terminal': Line 60: data['terminal'] = True Line 61: next(tokens) # swallow 'flowid' > an assert would be even better than a comment. Created special exception raising semantics. Line 62: next(tokens) # swallow '???' Line 63: elif token == 'ht': Line 64: next(tokens) Line 65: data['ht_divisor'] = next(tokens) Line 114: data['kind'] = next(tokens) Line 115: action_opt_parse = _ACTIONS.get(data['kind']) Line 116: if action_opt_parse is not None: Line 117: data.update(action_opt_parse(tokens)) Line 118: return data > a faulty input can make this function return None. Do we want that? Is a Va Done Line 119: Line 120: Line 121: def _parse_mirred(tokens): Line 122: """Parses the tokens of a mirred action into a data dictionary""" Line 124: action = next(tokens)[1:] # Get the first token without the opening paren Line 125: if action == 'unkown': Line 126: data['action'] = action Line 127: else: Line 128: data['action'] = '%s_%s' % (action.lower(), next(tokens).lower()) > Could you explain this case-insignificance? sure, I get the actions as 'Mirred' and 'Egress'. I don't want to have to remember that out of everything that is returned to me, the actions might have caps. Line 129: next(tokens) # swallow 'to' Line 130: next(tokens) # swallow 'device' Line 131: data['target'] = next(tokens)[:-1] Line 132: data['op'] = next(tokens) Line 129: next(tokens) # swallow 'to' Line 130: next(tokens) # swallow 'device' Line 131: data['target'] = next(tokens)[:-1] Line 132: data['op'] = next(tokens) Line 133: next(tokens) # pop the 0 that marks new line > assert that '0' is really seen here? Done Line 134: for token in tokens: Line 135: if token in ('index', 'ref', 'bind'): Line 136: data[token] = next(tokens) Line 137: elif token == 0: -- To view, visit http://gerrit.ovirt.org/29211 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I95aa9aaaeb0f8e612734b32a578e46fade7fdd9c Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Livnat Peer <[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
