Nir Soffer has posted comments on this change.
Change subject: libvirtconnection: Fix a race when starting the eventloop
......................................................................
Patch Set 3: Code-Review-1
(3 comments)
Lets make it simpler and fix the commit message.
....................................................
Commit Message
Line 6:
Line 7: libvirtconnection: Fix a race when starting the eventloop
Line 8:
Line 9: There is a race when starting the event loop and using the self.run
boolean.
Line 10: after the thread of libvirtEventLoop was already started.
This is a duplicate of the next paragraph - remove it.
Line 11:
Line 12: There is a race setting self.run to True after starting the thread,
Line 13: and checking if self.run is True from the thread main function.
Line 14:
Line 13: and checking if self.run is True from the thread main function.
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:
But in the patch you don't set run to true at the beginning of the method.
Describing how you fix the race here is a bad idea, forcing you now to update
the commit message each time the code changes. Why not simply:
This patch fixes this race.
Line 18: Signed-off-by: Maor Lipchuk <[email protected]>
....................................................
File lib/vdsm/libvirtconnection.py
Line 44: try:
Line 45: self.__thread.start()
Line 46: except Exception:
Line 47: self.run = False
Line 48: raise
Thread.start raise only when invoking it after the thread was started. Since we
create a new thread here, it cannot raise.
In the first version, when self.run was set in the beginning of the method,
using try except made sense, making the code more robust if you add more code
in this block. Now when we set self.run just before the thread is started,
there is no point in this try block. So the simplest solution that Dan
suggested is indeed equal to this code:
self.run = True
self.__thread.start()
Lets remove the unneeded complexity.
Line 49:
Line 50: def stop(self, wait=True):
Line 51: if self.run:
Line 52: self.run = False
--
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: 3
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
