Dan Kenigsberg has posted comments on this change.

Change subject: netlink: event monitor (even more simple version)
......................................................................


Patch Set 15: Code-Review-1

(9 comments)

http://gerrit.ovirt.org/#/c/36197/15/lib/vdsm/netlink/__init__.py
File lib/vdsm/netlink/__init__.py:

Line 67: 
Line 68: 
Line 69: def _open_socket(seq_check=True, callback_function=None, 
callback_arg=None):
Line 70:     """Returns an open netlink socket.
Line 71:         seq_check: If set to False, checking of sequence numbers on 
the netlink
I'd love to keep this text, but as a comment to _nl_socket_disable_seq_check(). 
We can disable it whenever callback_function is not None.
Line 72:         socket is disabled. This is required to allow messages to be 
processed
Line 73:         which were not requested by a preceding request message, e.g. 
netlink
Line 74:         events.
Line 75: 


Line 156: 
Line 157: 
Line 158: def _socket_memberships(socket_membership_function, socket, groups):
Line 159:     groups_codes = [_GROUPS[g] for g in groups]
Line 160:     groups_codes = groups_codes + [0] * (15 - len(groups_codes))
15 = len(_GROUPS) - 1
Line 161:     err = socket_membership_function(socket, *groups_codes)
Line 162:     if err:
Line 163:         _nl_socket_free(socket)
Line 164:         raise IOError(-err, _nl_geterror())


http://gerrit.ovirt.org/#/c/36197/15/lib/vdsm/netlink/monitor.py
File lib/vdsm/netlink/monitor.py:

Line 86:             pass
Line 87: 
Line 88:     def start(self):
Line 89:         if not self._running:
Line 90:             thread.start_new(self._scan, tuple())
please use the higher-level threading module, unless you have a good reason not 
to.
Line 91:             self._running = True
Line 92: 
Line 93:     def _scan(self):
Line 94:         epoll = select.epoll()


Line 97:                 with _pipetrick_w(epoll) as self._pipetrick:
Line 98:                     while True:
Line 99:                         events = NoIntrPoll(epoll.poll)
Line 100:                         if (self._pipetrick[0], select.POLLIN) in 
events:
Line 101:                             os.read(self._pipetrick[0], 1)
strictly speaking, this read should be wrapped with NoIntrCall, that should be 
moved to vdsm.utils (on another patch)
Line 102:                             break
Line 103:                         _nl_recvmsgs_default(sock)
Line 104:         finally:
Line 105:             epoll.close()


Line 152:     78: 'get_dcb',             # RTM_GETDCB
Line 153:     79: 'set_dcb'}             # RTM_SETDCB
Line 154: 
Line 155: 
Line 156: # This function serves as a callback for nl_msg_parse(message, 
callback,
please re-write as a proper docstring.
Line 157: # extra_argument) function. When nl_msg_parse() is called, it passes 
message as
Line 158: # an object to defined callback with optional extra argument 
(monitor's queue
Line 159: # in our case)
Line 160: def _object_input(obj, queue):


Line 181: # This function serves as a callback for netlink socket. When socket 
recieves
Line 182: # a message, it passes it to callback function with optional extra 
argument
Line 183: # (monitor's queue in this case)
Line 184: def _event_input(msg, queue):
Line 185:     errno = _nl_msg_parse(msg, _c_object_input, queue)
errno is an stdlib module, please do not override its name.
Line 186:     if errno < 0:
Line 187:         pass  # TODO: what to do?
Line 188:     return _NL_STOP
Line 189: _c_event_input = CFUNCTYPE(c_int, c_void_p, c_void_p)(_event_input)


Line 183: # (monitor's queue in this case)
Line 184: def _event_input(msg, queue):
Line 185:     errno = _nl_msg_parse(msg, _c_object_input, queue)
Line 186:     if errno < 0:
Line 187:         pass  # TODO: what to do?
I hope it is safe to call logging.error() here.

if not - use sys.stderr.write().
Line 188:     return _NL_STOP
Line 189: _c_event_input = CFUNCTYPE(c_int, c_void_p, c_void_p)(_event_input)
Line 190: 
Line 191: 


Line 189: _c_event_input = CFUNCTYPE(c_int, c_void_p, c_void_p)(_event_input)
Line 190: 
Line 191: 
Line 192: @contextmanager
Line 193: def _monitor_socket_w(queue, groups, epoll):
_monitoring_socket
Line 194:     c_queue = py_object(queue)
Line 195:     sock = _open_socket(seq_check=False, 
callback_function=_c_event_input,
Line 196:                         callback_arg=c_queue)
Line 197:     try:


Line 218:             yield pipetrick
Line 219:         finally:
Line 220:             epoll.unregister(pipetrick[0])
Line 221:     finally:
Line 222:         os.close(pipetrick[0])
close second fd, too.
Line 223: 
Line 224: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0f4fcfde87ad51eb832f54862371b4da1281826e
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Petr Horáček <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Petr Horáček <[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

Reply via email to