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

[Jim Fehlig]

On 08/18/2017 01:44 AM, Erik Skultety wrote:
> 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 :).

I think you were pretty clear, its just that I've been around libvirt a long 
time and don't recall hearing such a preference. I was simply double checking :-).

I've pushed this one after making the suggested change.

Regards,
Jim