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

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(a)suse.com&gt; >>> --- >>> >>> 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 :).