Re: [libvirt] [PATCH v2 0/5] Don't run whole sec driver in namespace
On 01/09/2017 07:58 AM, Michal Privoznik wrote:
v2 of:
https://www.redhat.com/archives/libvir-list/2016-December/msg00907.html
diff to v1:
- Pushed 1/6 from the original series as it was ACKed
- Added more comments
- Fixed return value of virSecurityDACTransactionAppend and
virSecuritySELinuxTransactionAppend
- Dropped ignore_value() around virThreadLocalSet()
- Unset thread local just in transactionCommit APIs
I have not implemented any rollback yet, but I've added comments
where the implementation should go ;-)
Michal Privoznik (5):
security_dac: Resolve virSecurityDACSetOwnershipInternal const
correctness
security driver: Introduce transaction APIs
security_dac: Implement transaction APIs
security_selinux: Implement transaction APIs
qemu: Use transactions from security driver
src/libvirt_private.syms | 3 +
src/qemu/qemu_driver.c | 25 ++--
src/qemu/qemu_security.c | 100 ++++---------
src/security/security_dac.c | 261 +++++++++++++++++++++++++++++++++-
src/security/security_driver.h | 9 ++
src/security/security_manager.c | 68 +++++++++
src/security/security_manager.h | 7 +-
src/security/security_selinux.c | 256 ++++++++++++++++++++++++++++++++-
src/security/security_stack.c | 49 +++++++
src/storage/storage_backend.h | 2 +-
src/storage/storage_backend_fs.c | 2 +-
src/storage/storage_backend_gluster.c | 2 +-
src/storage/storage_driver.c | 6 +-
src/storage/storage_driver.h | 4 +-
src/util/virstoragefile.c | 2 +-
src/util/virstoragefile.h | 2 +-
16 files changed, 703 insertions(+), 95 deletions(-)
BTW: Perhaps an existing bug... If *Launch calls SetAllLabel and fails
a -1 is returned... Once SetAllLabel succeeds, all returns afterwards
are -2. When the caller detects a -2 return, then it forces the relabel
in the exit path.
So if we have a partial success/failure, then do we really restore
anything? Should the setting of ret = -2 go before the call to
qemuSecuritySetAllLabel? Not related to this set of changes, but since
I was looking at the code...
I guess for now not having Rollback will be no worse than what exists.
It would seem to matter more on Set processing and much harder on
Restore processing (other than ignoring errors and trying to restore as
many as possible).
I think the Rollback thoughts came mostly because I started thinking
about other transaction applications which don't process "All", but
rather process "Some". Not rolling back on Abort or forcing a caller to
Rollback may be unexpected.
Consider that the virSecurity*TransactionRun could "save" the status of
the call to each list entry and then if there is a failure, go through
each successful one, recall the saved data label data, and restore it
(ok conceptually at least). Taking this path means not worrying about
the partial failure path I noted above. If ignoring failures on Restore
is important, then Start could save in list whether this was a "Set" or
"Restore" operation...
In any case, I'm not going to hold things up for a Rollback... As long
as the notes from patch 1 and 5 are addressed, then ACK series.
John