Dan Kenigsberg has submitted this change and it was merged.
Change subject: sp: Nest try-finally for temporary secure change
......................................................................
sp: Nest try-finally for temporary secure change
There were two instances where the same try-finally block was used for
both acquiring a lock and setting the pool as secure for a block of
code:
acquire lock
try:
set secure
code that needs both lock and secure state
finally:
set unsecure
release lock
This type of usage is incorrect. try-finally block should be used for
one change to ensure that the change is finally reverted, and to make
the intention of the code clear.
This patch use nested try-finally blocks, one for the lock, and one for
the secure state:
acquire lock
try:
set secure
try:
code that needs both lock and secure state
finally:
set unsecure
finally:
release lock
Because of the extra nesting, comments inside the nested code were
reformatted and missing punctuation was added.
I considered replacing the nested try-finally with a secured context
manager, but since the current code uses try-except-finally, using a
context manager would create an even deeper nesting or extracting some
new private methods. To keep minimal changes, I avoid this direction.
It seems that the secure state block can become smaller - some method
inside the secured block are @unsecured. I kept the secure block
semantics are they are now, and will check minimizing the secured block
in a later patch.
Change-Id: Ia9054063db0eeacd156a8e586681496515f80e2b
Signed-off-by: Nir Soffer <nsof...@redhat.com>
Reviewed-on: http://gerrit.ovirt.org/23955
Reviewed-by: Dan Kenigsberg <dan...@redhat.com>
Reviewed-by: Federico Simoncelli <fsimo...@redhat.com>
Reviewed-by: Ayal Baron <aba...@redhat.com>
Reviewed-by: Allon Mureinik <amure...@redhat.com>
---
M vdsm/storage/sp.py
1 file changed, 48 insertions(+), 48 deletions(-)
Approvals:
Ayal Baron: Looks good to me, approved
Nir Soffer: Verified
Federico Simoncelli: Looks good to me, but someone else must approve
Allon Mureinik: Looks good to me, but someone else must approve
Dan Kenigsberg: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.ovirt.org/23955
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia9054063db0eeacd156a8e586681496515f80e2b
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsof...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Eduardo <ewars...@gmail.com>
Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Itamar Heim <ih...@redhat.com>
Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com>
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
