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

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(a)redhat.com&gt; >>> --- >>> >>> 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(a)redhat.com&gt; > > Thanks > > Pavel > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list