Re: [libvirt] [PATCH 04/11] security: Remove security driver internals for disk labelling
On 1/23/19 11:10 AM, Peter Krempa wrote:
Security labelling of disks consists of labelling of the disk image
*labeling
itself and it's backing chain. Modify
virSecurityManager[Set|Restore]ImageLabel to take a boolean flag that
will label the full chain rather than the top image itself.
This allows to delete/unify some parts of the code and will also
simplify callers in some cases.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_security.c | 6 ++--
src/security/security_apparmor.c | 24 +++------------
src/security/security_dac.c | 40 +++++++------------------
src/security/security_driver.h | 15 +++-------
src/security/security_manager.c | 20 ++++++++-----
src/security/security_manager.h | 6 ++--
src/security/security_nop.c | 25 +++-------------
src/security/security_selinux.c | 42 ++++++++-------------------
src/security/security_stack.c | 50 +++++---------------------------
9 files changed, 60 insertions(+), 168 deletions(-)
I see two logical things happening...
1. Removing virSecurityDomain{Set|Restore}DiskLabel in favor of
virSecurityDomain{Set|Restore}ImageLabel
2. Adding parameter to virSecurityManager{Set|Restore}ImageLabel.
I think the parameter should be "unsigned int flags" instead of "bool
backingChain"? The latter is too specific. Then of course the first flag
defined is for backingChain. Also avoids some future change adding bool
myNewFlagName to the API. I do see a few other API's use bool's, but
does that mean they're right?
Beyond that - seems odd to remove the backend/inside of the
{Set|Restore}DiskLabel before replacing all the callers uses to
{Set|Restore}ImageLabel first. I see the bisect problem is handled by
changing virSecurityManager{Set|Restore}DiskLabel to call
domain{Set|Restore}SecurityImageLabel instead.
So, w/r/t commit message to "describe" what's happening consider
indicating the short term usage of *ImageLabel for the *DiskLabel. The
alternative is reordering patches, which I don't find necessary.
Assuming usage of @flags and I'm confident you can make that
alteration... So for the logic,
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
[...]