Code review comment for lp:~jameinel/launchpad/lp-service

Revision history for this message
Martin Pool (mbp) wrote :

That's a great improvement in time.

I think (per your decision 1) it would be good to change it to a unix-domain socket, for the sake of a bit more security, and better reusability as a per-user thing.

Per http://www.pathname.com/fhs/pub/fhs-2.3.html#VARRUNRUNTIMEVARIABLEDATA

> System programs that maintain transient UNIX-domain sockets must place them in this directory.

which will require sysadmins making a directory under there that's writable by this process. That can be in the config file. You should just need socket.bind((AF_UNIX, path)). Then just chmod the socket.

Having a config option to turn this on is great. We could make it active only on staging at first and see the effect.

481 + while not self._should_terminate.isSet():
482 + try:
483 + conn, client_addr = self._server_socket.accept()
484 + except self._socket_timeout:
485 + pass # run shutdown and children checks
486 + except self._socket_error, e:
487 + if e.args[0] == errno.EINTR:
488 + pass # run shutdown and children checks
489 + elif e.args[0] != errno.EBADF:
490 + # We can get EBADF here while we are shutting down
491 + # So we just ignore it for now
492 + pass

It seems like this might spin if the server socket's closed and there are no dead children.

+ fifos on the filesystem, and start running the requseted command. The

typo

We should do some kind of plan or review with LOSAs on how to deploy and support this. We should configure it to log to a file, and for that to be synced to devpad. Putting the pid in the log file would be nice.

« Back to merge proposal