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@xxxxxxxxxx>
>>> ---
>>>
>>> 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@xxxxxxxxxx>
>
> Thanks
>
> Pavel
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
