Change in vdsm[master]: virt: Handle fork() related issues in vmchannels/guestagent

nsoffer at redhat.com nsoffer at redhat.com
Tue Dec 15 16:13:29 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: virt: Handle fork() related issues in vmchannels/guestagent
......................................................................


Patch Set 12:

(2 comments)

https://gerrit.ovirt.org/#/c/42570/12/vdsm/virt/guestagent.py
File vdsm/virt/guestagent.py:

Line 223: 
Line 224:     @staticmethod
Line 225:     def _create_socket():
Line 226:         sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
Line 227:         GuestAgent._set_cloexec_flag(sock.fileno())
Better use vdsm.infra.filecontrol.set_close_on_exec()
Line 228:         sock.setblocking(0)
Line 229:         return sock
Line 230: 
Line 231:     @staticmethod


https://gerrit.ovirt.org/#/c/42570/12/vdsm/virt/vmchannels.py
File vdsm/virt/vmchannels.py:

Line 59:             # closed on fork we have to always unregister file descriptors
Line 60:             # in case of an error, no matter if we think that we're tracking
Line 61:             # them or not. Here however it is possible that epoll has already
Line 62:             # removed the file descriptor from the the internal set. That's
Line 63:             # why we have to handle the ENOENT case
I don't think we need this comment. Our approach should not be - we don't need to unregister because epoll automatically does it when file descriptor is closed, but we have special cases when we do have to unregister.

The safe and simple approach is - we always unregister before closing a descriptor and we don't care about epoll non standard behavior that does not help anyone.

We should have a helper method that unregister and close a socket, and call it from any code that does close() today.
Line 64:             try:
Line 65:                 self._epoll.unregister(fileno)
Line 66:             except IOError as e:
Line 67:                 if e.errno != errno.ENOENT:


-- 
To view, visit https://gerrit.ovirt.org/42570
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4a452f7837267434bc836fced4c2efd92d745cf
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Gal Hammer <ghammer%redhat.com at gtempaccount.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list