8:44 a.m.
Federico Simoncelli has uploaded a new change for review.
Change subject: BZ#807642 Use Securable as base class
......................................................................
BZ#807642 Use Securable as base class
Using Securable as base class allow us to take the best of metaclasses
(being able to modifying methods at import time) and the best of
subclassing (explicit inherited methods such as __init__, _setSafe and
_setUnsafe).
It's also now possible to instantiate attributes per object (__init__)
instead of per class and therefore fix BZ#807642.
Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
---
M vdsm/storage/securable.py
M vdsm/storage/sp.py
2 files changed, 44 insertions(+), 44 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/98/3198/1
--
To view, visit http://gerrit.ovirt.org/3198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
2:56 p.m.
Ayal Baron has posted comments on this change.
Change subject: BZ#807642 Use Securable as base class
......................................................................
Patch Set 3: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.ovirt.org/3198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
8:02 a.m.
Dan Kenigsberg has submitted this change and it was merged.
Change subject: BZ#807642 Use Securable as base class
......................................................................
BZ#807642 Use Securable as base class
Using Securable as base class allow us to take the best of metaclasses
(being able to modifying methods at import time) and the best of
subclassing (explicit inherited methods such as __init__, _setSafe and
_setUnsafe).
It's also now possible to instantiate attributes per object (__init__)
instead of per class and therefore fix BZ#807642.
Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
---
M vdsm/storage/securable.py
M vdsm/storage/sp.py
2 files changed, 34 insertions(+), 34 deletions(-)
Approvals:
Ayal Baron: Looks good to me, but someone else must approve
Saggi Mizrahi: Looks good to me, but someone else must approve
Dan Kenigsberg: Verified; Looks good to me, approved
--
To view, visit http://gerrit.ovirt.org/3198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
8:01 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: BZ#807642 Use Securable as base class
......................................................................
Patch Set 3: Verified
<hateya> its working
--
To view, visit http://gerrit.ovirt.org/3198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
4:15 a.m.
Federico Simoncelli has posted comments on this change.
Change subject: BZ#807642 Use Securable as base class
......................................................................
Patch Set 3: (1 inline comment)
....................................................
File vdsm/storage/securable.py
Line 60: if not (self._isSafe() or override):
Even if fixed, someone can call self._setUnsafe while f() is running,
and nothing in Securable protects f from doing harm.
The idea here is to be consistent in the positive flow. The self._setUnsafe()
shouldn't complete until f() completed. Nothing in securable protects from f(), that
is correct, but we shouldn't advertise that we stopped the spm while we still have f()
running. That is why we would need to queue stopSpm behind the secured calls.
I agree on postponing this fix.
--
To view, visit http://gerrit.ovirt.org/3198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
4:10 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: BZ#807642 Use Securable as base class
......................................................................
Patch Set 3: (1 inline comment)
....................................................
File vdsm/storage/securable.py
Line 60: if not (self._isSafe() or override):
I think that the problem with Securable runs even deeper than this race. Even if fixed,
someone can call self._setUnsafe while f() is running, and nothing in Securable protects f
from doing harm.
spmStop makes sure to not only _setUnsafe, but umount the master filesystem so we have
some additional protection.
In any case, this issue should not be mixed with the current patch.
--
To view, visit http://gerrit.ovirt.org/3198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
3:06 a.m.
Federico Simoncelli has posted comments on this change.
Change subject: BZ#807642 Use Securable as base class
......................................................................
Patch Set 3: (1 inline comment)
....................................................
File vdsm/storage/securable.py
Line 60: if not (self._isSafe() or override):
Dan do you want me to get this race too? The solution is not completely trivial, it might
require a rwlock to avoid the serialization of all the calls.
--
To view, visit http://gerrit.ovirt.org/3198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
9:26 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: BZ#807642 Use Securable as base class
......................................................................
Patch Set 3: Looks good to me, approved
I've asked preinteg655 to verify this
--
To view, visit http://gerrit.ovirt.org/3198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
1:12 p.m.
Saggi Mizrahi has posted comments on this change.
Change subject: BZ#807642 Use Securable as base class
......................................................................
Patch Set 3: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.ovirt.org/3198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
8:45 a.m.
Federico Simoncelli has posted comments on this change.
Change subject: BZ#807642 Use Securable as base class
......................................................................
Patch Set 1:
Proof of concept. Tested locally with some example classes, but not tested with
StoragePool (yet).
--
To view, visit http://gerrit.ovirt.org/3198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2309e2cfc0291309023238ed91527884ec879d64
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
4407
days inactive
4411
days old
vdsm-patches@lists.fedorahosted.org
9 comments
4 participants
participants (4)
-
abaron＠redhat.com -
Dan Kenigsberg -
Federico Simoncelli -
smizrahi＠redhat.com