Antoni Segura Puimedon has posted comments on this change. Change subject: netlink: event monitor ......................................................................
Patch Set 20: Code-Review-1 (17 comments) http://gerrit.ovirt.org/#/c/32626/20/lib/vdsm/netlink/__init__.py File lib/vdsm/netlink/__init__.py: Line 73: if err: Line 74: _nl_socket_free(sock) Line 75: raise IOError(-err, _nl_geterror()) Line 76: Line 77: if callback_function: is not None Line 78: err = _nl_socket_modify_cb(sock, _NL_CB_DEFAULT, _NL_CB_CUSTOM, Line 79: callback_function, None) Line 80: Line 81: err = _nl_connect(sock, _NETLINK_ROUTE) Line 145: Line 146: def _get_groups_codes(groups): Line 147: groups_codes = [] Line 148: for g in groups: Line 149: code = _KNOWN_GROUPS.get(g, None) .get already has None as default value, no need to specify Line 150: if not code: Line 151: raise UnknownGroup Line 152: groups_codes.append(code) Line 153: return groups_codes Line 146: def _get_groups_codes(groups): Line 147: groups_codes = [] Line 148: for g in groups: Line 149: code = _KNOWN_GROUPS.get(g, None) Line 150: if not code: if code is None Line 151: raise UnknownGroup Line 152: groups_codes.append(code) Line 153: return groups_codes Line 154: Line 186: Line 187: _nl_socket_alloc = CFUNCTYPE(c_void_p)(('nl_socket_alloc', LIBNL)) Line 188: _nl_socket_free = _none_proto(('nl_socket_free', LIBNL)) Line 189: Line 190: _nl_msg_parse = _int_proto(('nl_msg_parse', LIBNL)) int nlmsg_parse(struct nlmsghdr * nlh, int hdrlen, struct nlattr * tb[], int maxtype, struct nla_policy * policy) So CFUNCTYPE for returning int and passing all these params above. Line 191: _nl_socket_add_memberships = _int_proto(('nl_socket_add_memberships', Line 192: LIBNL)) Line 193: _nl_socket_disable_seq_check = _int_proto(('nl_socket_disable_seq_check', Line 194: LIBNL)) Line 188: _nl_socket_free = _none_proto(('nl_socket_free', LIBNL)) Line 189: Line 190: _nl_msg_parse = _int_proto(('nl_msg_parse', LIBNL)) Line 191: _nl_socket_add_memberships = _int_proto(('nl_socket_add_memberships', Line 192: LIBNL)) this should have its own C prototype as it the C signature is: int nl_socket_add_memberships(struct nl_sock * sk, int group, ... ) So you need the CFUNCTYPE with varargs of int type. Line 193: _nl_socket_disable_seq_check = _int_proto(('nl_socket_disable_seq_check', Line 194: LIBNL)) Line 195: _nl_socket_drop_memberships = _int_proto(('nl_socket_drop_memberships', Line 196: LIBNL)) Line 196: LIBNL)) Line 197: _nl_socket_get_fd = _int_proto(('nl_socket_get_fd', LIBNL)) Line 198: _nl_socket_modify_cb = _int_proto(('nl_socket_modify_cb', LIBNL)) Line 199: _nl_object_get_type = _char_proto(('nl_object_get_type', LIBNL)) Line 200: _nl_recvmsgs_default = _int_proto(('nl_recvmsgs_default', LIBNL)) Review all of them. Line 201: Line 202: _nl_connect = CFUNCTYPE(c_int, c_void_p, c_int)(('nl_connect', LIBNL)) Line 203: _nl_geterror = CFUNCTYPE(c_char_p)(('nl_geterror', LIBNL)) Line 204: http://gerrit.ovirt.org/#/c/32626/20/lib/vdsm/netlink/monitor.py File lib/vdsm/netlink/monitor.py: Line 29: from addr import _addr_info Line 30: from link import _link_info, _rtnl_link_alloc_cache Line 31: from route import _route_info Line 32: Line 33: _STOP_QUEUE = 80 There should be an inline comment for knowing what this is. Line 34: Line 35: Line 36: class MonitorError(Exception): Line 37: pass Line 65: decnet-ifaddr, decnet-route, ipv6-prefix Line 66: """ Line 67: def __init__(self): Line 68: self.queue = Queue.Queue() Line 69: self.thread = MonitorThread(self) self._queue self._thread Don't pass self to the monitor Thread. better pass the Queue and other things it needs here. Line 70: Line 71: def __iter__(self): Line 72: try: Line 73: while True: Line 70: Line 71: def __iter__(self): Line 72: try: Line 73: while True: Line 74: if not self.thread.is_alive(): First process if it is alive, then the else. Line 75: yield self.queue.get_nowait() Line 76: else: Line 77: value = self.queue.get() Line 78: if value == _STOP_QUEUE: Line 89: threading.Thread.__init__(self.thread) Line 90: if not groups or 'all' in groups: Line 91: self.groups = _KNOWN_GROUPS.keys() Line 92: else: Line 93: groups Remove! Line 94: self.groups = list(groups) Line 95: if timeout: Line 96: self.timeout = timeout Line 97: self.timeStart = time.time() Line 90: if not groups or 'all' in groups: Line 91: self.groups = _KNOWN_GROUPS.keys() Line 92: else: Line 93: groups Line 94: self.groups = list(groups) remove "list" casting Line 95: if timeout: Line 96: self.timeout = timeout Line 97: self.timeStart = time.time() Line 98: Line 92: else: Line 93: groups Line 94: self.groups = list(groups) Line 95: if timeout: Line 96: self.timeout = timeout Move the configurability (groups, timeout, etc) to the __init__. Let the users create more MOnitors. Line 97: self.timeStart = time.time() Line 98: Line 99: self.thread.start() Line 100: Line 110: Line 111: Line 112: class MonitorThread(threading.Thread): Line 113: def __init__(self, monitor): Line 114: threading.Thread.__init__(self) check 'super' out. Line 115: self.daemon = True Line 116: self.monitor = monitor Line 117: self.stop = threading.Event() Line 118: Line 112: class MonitorThread(threading.Thread): Line 113: def __init__(self, monitor): Line 114: threading.Thread.__init__(self) Line 115: self.daemon = True Line 116: self.monitor = monitor this out Line 117: self.stop = threading.Event() Line 118: Line 119: def run(self): Line 120: self.stop.clear() Line 118: Line 119: def run(self): Line 120: self.stop.clear() Line 121: Line 122: def _object_input(obj): Move to the module level and make in __init__ a partial that passes it the queue it needs. (and in makes the CFUNCTYPE to it (cause it is possibly not _int_proto). Line 123: obj_type = _nl_object_get_type(obj) Line 124: if obj_type == _NL_ROUTE_ADDR_NAME: Line 125: self.monitor.queue.put(_addr_info(obj, self.cache)) Line 126: return 0 Line 127: elif obj_type == _NL_ROUTE_LINK_NAME: Line 128: self.monitor.queue.put(_link_info(obj, self.cache)) Line 129: return 0 Line 130: elif obj_type.split('/', 1)[0] == _NL_ROUTE_NAME: Line 131: self.monitor.queue.put(_route_info(obj, self.cache)) Instead of putting in the queue, assign to 'object_dict'. Then call nl_object_get_msgtype on 'obj' to retrieve the netlink message integer that originated the object. Convert it to string via 'nl_nlmsgtype2str' and put that into the queue with key 'msg_type'. Line 132: return 0 Line 133: Line 134: def _event_input(msg): Line 135: _nl_msg_parse(msg, _c_object_input, None) Line 140: Line 141: try: Line 142: self.sock = _open_socket(seq_check=False, Line 143: callback_function=_c_event_input) Line 144: self.cache = _rtnl_link_alloc_cache(self.sock) The problem with using a cache is that you would have to maintain it up to date. Either do that, or let's work without it (if an item is deleted, i.e., you get IOError ENODEV we just report it without label). Line 145: Line 146: _add_socket_memberships(self.sock, self.monitor.groups) Line 147: while not self.stop.is_set(): Line 148: if self.monitor.timeout: -- To view, visit http://gerrit.ovirt.org/32626 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I23ea72986564c5a115e36be0e7cf679c28c8ea96 Gerrit-PatchSet: 20 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Petr Horáček <[email protected]> Gerrit-Reviewer: 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
