Adam Litke has posted comments on this change. Change subject: storage: Introduce guarded utilities ......................................................................
Patch Set 6: (4 comments) https://gerrit.ovirt.org/#/c/61435/6/tests/storagetestlib.py File tests/storagetestlib.py: Line 367: def calls(self): Line 368: return [c[1] for c in self._calls] Line 369: Line 370: def acquire(self, name): Line 371: self._calls.append(('a', name)) > Using 'acquire' and 'release' instead of single letters would make the code Done Line 372: Line 373: def release(self, name): Line 374: self._calls.append(('r', name)) Line 375: Line 373: def release(self, name): Line 374: self._calls.append(('r', name)) Line 375: Line 376: def validate(self, locks): Line 377: calls = [('a', l) for l in locks] + [('r', l) for l in reversed(locks)] > Using "name" instead of "l" will make this much more clear, and also match Done Line 378: if calls != self._calls: Line 379: raise AssertionError("Locking order validation failed: " Line 380: "expecting %r, got %r", calls, self._calls) Line 381: Line 401: def acquire(self): Line 402: pass Line 403: Line 404: def release(self): Line 405: pass > Why not keep the locking state? We will use a log instead so we can also check ordering. Line 406: Line 407: Line 408: @contextmanager Line 409: def checked_locks(): Line 420: (guarded.ResourceManagerLock, 'acquire', checked_acquire), Line 421: (guarded.ResourceManagerLock, 'release', checked_release), Line 422: (guarded.VolumeLease, 'acquire', checked_acquire), Line 423: (guarded.VolumeLease, 'release', checked_release), Line 424: ]): > This looks too complicated - why do we need to mock a fake lock? We don't now. Only mocking the real locks now. -- To view, visit https://gerrit.ovirt.org/61435 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2b0a204818d44b6205515277f4c2834cb2b7a057 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Freddy Rolland <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
