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