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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
>
> John
>
> [...]
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
