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

[Pavel Hrdina]

On Thu, Jun 15, 2017 at 06:50:34AM -0400, John Ferlan wrote:
> 
> 
> 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.

Oh right.  The only thing what that check currently does is that for
smartcard, rng and redirdev it skips parsing of the seclabel.  I would
probably leave it to a separate patch which would ensure that we don't
start parsing the seclabel for these devices.

Pavel

> 
> John
> 
> >> The rest looks good, so
> >>
> >> Reviewed-by: John Ferlan <jferlan at redhat.com>
> > 
> > Thanks
> > 
> > Pavel
> > 
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170615/c13f4e2a/attachment-0001.sig>