[libvirt] [PATCH v2 1/4] conf: move seclabel for chardev source to the correct sturcture

[John Ferlan]

On 06/15/2017 02:39 AM, Pavel Hrdina wrote:
> On Tue, Jun 13, 2017 at 12:35:10PM -0400, John Ferlan wrote:
>>
>>
>> On 05/29/2017 10:31 AM, Pavel Hrdina wrote:
>>> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
>>> ---
>>>
>>> Notes:
>>>     new in v2
>>>
>>>  src/conf/domain_conf.c          | 46 +++++++++++++++++++----------------------
>>>  src/conf/domain_conf.h          |  9 ++++----
>>>  src/security/security_dac.c     | 26 ++++++++++-------------
>>>  src/security/security_manager.c |  4 ++--
>>>  src/security/security_selinux.c | 24 +++++++++------------
>>>  5 files changed, 49 insertions(+), 60 deletions(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index c7e20b8ba1..68dc2832cb 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -2076,12 +2076,21 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
>>>  
>>>  void virDomainChrSourceDefFree(virDomainChrSourceDefPtr def)
>>>  {
>>> +    size_t i;
>>> +
>>>      if (!def)
>>>          return;
>>>  
>>>      virDomainChrSourceDefClear(def);
>>>      virObjectUnref(def->privateData);
>>>  
>>> +    if (def->seclabels) {
>>> +        for (i = 0; i < def->nseclabels; i++)
>>> +            virSecurityDeviceLabelDefFree(def->seclabels[i]);
>>> +        VIR_FREE(def->seclabels);
>>> +    }
>>> +
>>> +
>>>      VIR_FREE(def);
>>>  }
>>>  
>>> @@ -2150,8 +2159,6 @@ virDomainChrSourceDefIsEqual(const virDomainChrSourceDef *src,
>>>  
>>>  void virDomainChrDefFree(virDomainChrDefPtr def)
>>>  {
>>> -    size_t i;
>>> -
>>>      if (!def)
>>>          return;
>>>  
>>> @@ -2176,12 +2183,6 @@ void virDomainChrDefFree(virDomainChrDefPtr def)
>>>      virDomainChrSourceDefFree(def->source);
>>>      virDomainDeviceInfoClear(&def->info);
>>>  
>>> -    if (def->seclabels) {
>>> -        for (i = 0; i < def->nseclabels; i++)
>>> -            virSecurityDeviceLabelDefFree(def->seclabels[i]);
>>> -        VIR_FREE(def->seclabels);
>>> -    }
>>> -
>>>      VIR_FREE(def);
>>>  }
>>>  
>>> @@ -10688,8 +10689,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>>>                  if (chr_def) {
>>
>> Is the @chr_def check necessary still?  I assume it's there because
>> chr_def can be passed as NULL in some cases.
>>
>> Looks like all this was added as part of commit 'f8b08d0e'
>>
>> The chr_def would be NULL for Smartcard, RNG, and Redirdev callers which
>> each now doesn't pass a NULL to virDomainChrSourceDefFormat, so that
>> leads me to believe that the @chr_def should be removed.
> 
> But this hunk is from virDomainChrSourceDefParseXML() function.
> 

Yes, I knew that when I wrote the comment, but that wasn't the point.

Since you've moved the labels into @def instead and similarly altered
calls to virDomainChrSourceDefFormat such that they don't receive
chr_def (in the very next hunk of changes BTW), then I would think at
this point removing chr_def would be safe, but I suppose I could be
wrong. Hence why I asked.

So does it hurt to keep it, probably not; however, IIUC the reason why
it was there was because if it wasn't, then dereferencing chr_def to get
the [n]seclabels in the subsequent call wouldn't end well.

John

>> The rest looks good, so
>>
>> Reviewed-by: John Ferlan <jferlan at redhat.com>
> 
> Thanks
> 
> Pavel
>