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