[libvirt] [PATCH 04/11] security: Remove security driver internals for disk labelling

[Peter Krempa]

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 at 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 at redhat.com>
> 
> John
> 
> [...]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190130/8bd0106e/attachment-0001.sig>