On 05/03/2011 03:10 PM, Eric Blake wrote: > > Detected by clang. > > * src/esx/esx_driver.c (esxDomainGetInfo): Fail early on error. > --- >  src/esx/esx_driver.c |    1 + >  1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c > index 1f8f90b..e929208 100644 > --- a/src/esx/esx_driver.c > +++ b/src/esx/esx_driver.c > @@ -2372,8 +2372,9 @@ esxDomainGetInfo(virDomainPtr domain, > virDomainInfoPtr info) > >                  if (perfEntityMetric == NULL) { >                      VIR_ERROR(_("QueryPerf returned object with > unexpected type '%s'"), > >  esxVI_Type_ToString(perfEntityMetricBase->_type)); > +                    goto cleanup; >                  } > >                  perfMetricIntSeries = > >  esxVI_PerfMetricIntSeries_DynamicCast(perfEntityMetric->value); I would just say ACK, since this obviously eliminates a null dereference, but I notice that the following check for perfMetricIntSeries == NULL also calls VIR_ERROR and then doesn't goto cleanup, so I'm wondering if maybe the intent is that if either of these is NULL, result should still get set to 0. Mathias?

On Wed, May 04, 2011 at 08:38:43AM +0200, Matthias Bolte wrote: > 2011/5/4 Laine Stump <laine(a)laine.org&gt;: > > On 05/03/2011 03:10 PM, Eric Blake wrote: > >> > >> Detected by clang. > >> > >> * src/esx/esx_driver.c (esxDomainGetInfo): Fail early on error. > >> --- > >>  src/esx/esx_driver.c |    1 + > >>  1 files changed, 1 insertions(+), 0 deletions(-) > >> > >> diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c > >> index 1f8f90b..e929208 100644 > >> --- a/src/esx/esx_driver.c > >> +++ b/src/esx/esx_driver.c > >> @@ -2372,8 +2372,9 @@ esxDomainGetInfo(virDomainPtr domain, > >> virDomainInfoPtr info) > >> > >>                  if (perfEntityMetric == NULL) { > >>                      VIR_ERROR(_("QueryPerf returned object with > >> unexpected type '%s'"), > >> > >>  esxVI_Type_ToString(perfEntityMetricBase->_type)); > >> +                    goto cleanup; > >>                  } > >> > >>                  perfMetricIntSeries = > >> > >>  esxVI_PerfMetricIntSeries_DynamicCast(perfEntityMetric->value); > > > > I would just say ACK, since this obviously eliminates a null dereference, > > but I notice that the following check for perfMetricIntSeries == NULL also > > calls VIR_ERROR and then doesn't goto cleanup, so I'm wondering if maybe the > > intent is that if either of these is NULL, result should still get set to 0. > > Mathias? > > > > NACK, as written. > > There is a potential NULL dereference in there, but just going to > cleanup results in freeing static strings here. Patch 1 fixes it > correctly. > > Actually this code has been there for a while now, but didn't do > anything useful with the queried values because of the format mismatch > between libvirt and ESX. Therefore, patch 2 disables that code but > keeps it as a reference for how to query performance counters.  If code is not used, then I would ACK disabling for 0.9.1 and then we can figure out if this really should eb provided in some other ways. My only worry is a possible regression, you decide :-) Daniel