Change in vdsm[master]: libvirtconnection: Fix a race when starting an eventloop

nsoffer at redhat.com nsoffer at redhat.com
Tue Dec 31 21:01:35 UTC 2013


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 <mlipchuk at redhat.com>
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 <mlipchuk at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipchuk at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list