Dan Kenigsberg has posted comments on this change. Change subject: tc: replace mirred filter parsing with a generic filter parser ......................................................................
Patch Set 6: Code-Review-1 (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 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 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 or line.startswith('\t') to the following line. 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 it clearer. 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 of argument in the docstring. and more importantly - it's a re-implementation of the iter() builtin. 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 newcomer) to understand the intricacies of this parsing. 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. 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 ValueError due here? 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? 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? 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: 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
