On Mon, Jan 28, 2019 at 09:26:45 -0500, John Ferlan wrote:
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@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
I'll use the correct type here.
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?
It will be somewhat verbose: +typedef enum { + VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN = 1 << 0; +} virSecurityDomainImageLabelFlags; + typedef int (*virSecurityDomainSetImageLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, virStorageSourcePtr src, - bool backingChain); + virSecurityDomainImageLabelFlags flags); typedef int (*virSecurityDomainRestoreImageLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, virStorageSourcePtr src, - bool backingChain); + virSecurityDomainImageLabelFlags flags; typedef int (*virSecurityDomainSetMemoryLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainMemoryDefPtr mem);
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.
It's not only "bisect problem" but genuine replacement of the internals with a different internal impl without changing the callers. The disk labelling function becomes a shim which adds a parameter. This was done to limit scope change. This patch should in all callers besides the ones in Set|RestoreDisk label add the 'false' flag (or the equivalent.
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.
Reordering is impossible without adding the parameter first or squashing them together, where it would drag in semantic changes from the places calling {Set|Restore}DiskLabel.
Assuming usage of @flags and I'm confident you can make that alteration... So for the logic,
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
[...]