Nir Soffer has posted comments on this change. Change subject: libvirtconnection: Fix a race when starting an eventloop ......................................................................
Patch Set 2: (5 comments) .................................................... Commit Message Line 3: AuthorDate: 2013-12-31 16:42:56 +0200 Line 4: Commit: Maor Lipchuk <[email protected]> Line 5: CommitDate: 2013-12-31 11:45:32 -0500 Line 6: Line 7: libvirtconnection: Fix a race when starting an eventloop ... the event loop Line 8: Line 9: There is a race when starting an event loop and using the self.run boolean. Line 10: after the thread of libvirtEventLoop was already started. Line 11: Line 5: CommitDate: 2013-12-31 11:45:32 -0500 Line 6: Line 7: libvirtconnection: Fix a race when starting an eventloop Line 8: Line 9: There is a race when starting an event loop and using the self.run boolean. ... the even loop. Line 10: after the thread of libvirtEventLoop was already started. Line 11: Line 12: The libvirtEventLoop is counting on the self.run to be true (in the while loop), Line 13: although the run is only changed to true after this method is called. Line 9: There is a race when starting an event loop and using the self.run boolean. Line 10: after the thread of libvirtEventLoop was already started. Line 11: Line 12: The libvirtEventLoop is counting on the self.run to be true (in the while loop), Line 13: although the run is only changed to true after this method is called. I think the description is not clear - how about: There is a race setting self.run to True after starting the thread, and checking if self.run is True from the thread main function. And it is important to describe the consequences of running without a libvirt event loop. Line 14: Line 15: The proposed fix is changing the run to true at the beginning of the method Line 16: and only change it back to false if we encounter an exception Line 17: or if the thread exits abnormally. .................................................... File lib/vdsm/libvirtconnection.py Line 40 Line 41 Line 42 Line 43 Line 44 But then if start() raises we are left with non-running thread and wrong self.run value. Of course start should not raise because this is always the first call on a new thread instance. Using the suggested patch we don't need to assume anything about Thread exception behavior, and the code is more robust for future changes, like adding another call in this block. I this that less fragile code is better then less code. Line 55: self.__thread = None Line 56: Line 57: @utils.traceback(on=log.name) Line 58: def __run(self): Line 59: try: Lets split this to another patch, as this is not related to the race. Line 60: libvirt.virEventRegisterDefaultImpl() Line 61: while self.run: Line 62: libvirt.virEventRunDefaultImpl() Line 63: finally: -- To view, visit http://gerrit.ovirt.org/22859 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7cad0adfdd6c10e173888ee95ef822e1a62f7767 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[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
