[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]