[libvirt] [PATCH V2 2/2] Don't autogenerate seclabels of type 'none'

[Erik Skultety]

On Fri, Aug 18, 2017 at 12:18:20AM -0600, Jim Fehlig wrote:
> On 08/17/2017 02:17 AM, Erik Skultety wrote:
> > On Wed, Aug 16, 2017 at 05:54:08PM -0600, Jim Fehlig wrote:
> > > When security drivers are active but confinement is not enabled,
> > > there is no need to autogenerate <seclabel> elements when starting
> > > a domain def that contains no <seclabel> elements. In fact,
> > > autogenerating the elements can result in needless save/restore and
> > > migration failures when the security driver is not active on the
> > > restore/migration target.
> > >
> > > This patch changes the virSecurityManagerGenLabel function in
> > > src/security_manager.c to only autogenerate a <seclabel> element
> > > if none is already defined for the domain *and* default
> > > confinement is enabled. Otherwise the needless <seclabel>
> > > autogeneration is skipped.
> > >
> > > Resolves: https://bugzilla.opensuse.org/show_bug.cgi?id=1051017
> > > Signed-off-by: Jim Fehlig <jfehlig at suse.com>
> > > ---
> > >
> > > V2: Don't autogenerate a seclabel if domain does not contain one
> > >      and confinement is disabled.
> > >
> > >   src/security/security_manager.c | 42 +++++++++++++++++++++--------------------
> > >   1 file changed, 22 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> > > index 013bbc37e..10515c314 100644
> > > --- a/src/security/security_manager.c
> > > +++ b/src/security/security_manager.c
> > > @@ -650,30 +650,32 @@ virSecurityManagerGenLabel(virSecurityManagerPtr mgr,
> > >       for (i = 0; sec_managers[i]; i++) {
> > >           generated = false;
> > >           seclabel = virDomainDefGetSecurityLabelDef(vm, sec_managers[i]->drv->name);
> > > -        if (!seclabel) {
> > > -            if (!(seclabel = virSecurityLabelDefNew(sec_managers[i]->drv->name)))
> > > -                goto cleanup;
> > > -            generated = seclabel->implicit = true;
> > > -        }
> > > +        if (seclabel) {
> >
> > Just a tiny nitpick, generally we prefer the 'if' block to be shorter than the
> > corresponding 'else' block.
>
> Thanks for the review! Sorry, but I'm not sure what you mean by "shorter".
> Do you mean the 'if' block should contain fewer lines of code than the
> 'else' block?

Yeah, that's exactly what I meant, I'm sorry, not being a native speaker once
again resulted in a poor choice of wording :/. Next time I'll do better in
expressing myself more precisely, I promise :).

Erik

>
> Regards,
> Jim