2011/5/4 Laine Stump <laine@xxxxxxxxx>:
> 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.
Matthias
From 6e9471c7aabf03a6ab5dedb29ec411aa683b8e44 Mon Sep 17 00:00:00 2001
From: Matthias Bolte <matthias.bolte@xxxxxxxxxxxxxx>
Date: Wed, 4 May 2011 08:27:57 +0200
Subject: [PATCH] esx: Avoid null dereference on error in esxDomainGetInfo
Add missing early exits and convert error logging to proper API level
error reporting.
Centralize cleanup code for the PerfQuerySpec object.
Reported by Eric Blake, detected by clang.
---
src/esx/esx_driver.c | 28 +++++++++++++++++++---------
1 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index 1f8f90b..5464bc0 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -2356,9 +2356,6 @@ esxDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
if (esxVI_QueryPerf(priv->host, querySpec,
&perfEntityMetricBaseList) < 0) {
- querySpec->entity = NULL;
- querySpec->metricId->instance = NULL;
- querySpec->format = NULL;
goto cleanup;
}
@@ -2371,16 +2368,20 @@ esxDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
esxVI_PerfEntityMetric_DynamicCast(perfEntityMetricBase);
if (perfEntityMetric == NULL) {
- VIR_ERROR(_("QueryPerf returned object with unexpected type '%s'"),
+ ESX_ERROR(VIR_ERR_INTERNAL_ERROR,
+ _("QueryPerf returned object with unexpected type '%s'"),
esxVI_Type_ToString(perfEntityMetricBase->_type));
+ goto cleanup;
}
perfMetricIntSeries =
esxVI_PerfMetricIntSeries_DynamicCast(perfEntityMetric->value);
if (perfMetricIntSeries == NULL) {
- VIR_ERROR(_("QueryPerf returned object with unexpected type '%s'"),
+ ESX_ERROR(VIR_ERR_INTERNAL_ERROR,
+ _("QueryPerf returned object with unexpected type '%s'"),
esxVI_Type_ToString(perfEntityMetric->value->_type));
+ goto cleanup;
}
for (; perfMetricIntSeries != NULL;
@@ -2395,10 +2396,6 @@ esxDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
}
}
- querySpec->entity = NULL;
- querySpec->metricId->instance = NULL;
- querySpec->format = NULL;
-
VIR_DEBUG("usedCpuTimeCounterId %d END", priv->usedCpuTimeCounterId);
/*
@@ -2411,6 +2408,19 @@ esxDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
result = 0;
cleanup:
+ /*
+ * Remove values owned by data structures to prevent them from being freed
+ * by the call to esxVI_PerfQuerySpec_Free().
+ */
+ if (querySpec != NULL) {
+ querySpec->entity = NULL;
+ querySpec->format = NULL;
+
+ if (querySpec->metricId != NULL) {
+ querySpec->metricId->instance = NULL;
+ }
+ }
+
esxVI_String_Free(&propertyNameList);
esxVI_ObjectContent_Free(&virtualMachine);
esxVI_PerfMetricId_Free(&perfMetricIdList);
--
1.7.0.4
From 19524184a1d7dfa0b7140a398d0b0fcc7d75ecc6 Mon Sep 17 00:00:00 2001
From: Matthias Bolte <matthias.bolte@xxxxxxxxxxxxxx>
Date: Wed, 4 May 2011 08:34:31 +0200
Subject: [PATCH] esx: Disable performance counter queries in esxDomainGetInfo
The queried values aren't used yet.
---
src/esx/esx_driver.c | 17 +++++++++++++++++
1 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index 5464bc0..c958197 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -2163,6 +2163,15 @@ esxDomainSetMemory(virDomainPtr domain, unsigned long memory)
+/*
+ * libvirt exposed virtual CPU usage in absolute time, ESX doesn't provide this
+ * information in this format. It exposes it in 20 seconds slots, but it's hard
+ * to get a reliable absolute time from this. Therefore, disable the code that
+ * queries the performance counters here for now, but keep it as example for how
+ * to query a selected performance counter for its values.
+ */
+#define ESX_QUERY_FOR_USED_CPU_TIME 0
+
static int
esxDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
{
@@ -2173,6 +2182,7 @@ esxDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
esxVI_DynamicProperty *dynamicProperty = NULL;
esxVI_VirtualMachinePowerState powerState;
int64_t memory_limit = -1;
+#if ESX_QUERY_FOR_USED_CPU_TIME
esxVI_PerfMetricId *perfMetricId = NULL;
esxVI_PerfMetricId *perfMetricIdList = NULL;
esxVI_Int *counterId = NULL;
@@ -2185,6 +2195,7 @@ esxDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
esxVI_PerfEntityMetric *perfEntityMetric = NULL;
esxVI_PerfMetricIntSeries *perfMetricIntSeries = NULL;
esxVI_Long *value = NULL;
+#endif
memset(info, 0, sizeof (*info));
@@ -2249,6 +2260,7 @@ esxDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
/* memory_limit < 0 means no memory limit is set */
info->memory = memory_limit < 0 ? info->maxMem : memory_limit;
+#if ESX_QUERY_FOR_USED_CPU_TIME
/* Verify the cached 'used CPU time' performance counter ID */
/* FIXME: Currently no host for a vpx:// connection */
if (priv->host != NULL) {
@@ -2404,10 +2416,12 @@ esxDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
*/
}
}
+#endif
result = 0;
cleanup:
+#if ESX_QUERY_FOR_USED_CPU_TIME
/*
* Remove values owned by data structures to prevent them from being freed
* by the call to esxVI_PerfQuerySpec_Free().
@@ -2420,14 +2434,17 @@ esxDomainGetInfo(virDomainPtr domain, virDomainInfoPtr info)
querySpec->metricId->instance = NULL;
}
}
+#endif
esxVI_String_Free(&propertyNameList);
esxVI_ObjectContent_Free(&virtualMachine);
+#if ESX_QUERY_FOR_USED_CPU_TIME
esxVI_PerfMetricId_Free(&perfMetricIdList);
esxVI_Int_Free(&counterIdList);
esxVI_PerfCounterInfo_Free(&perfCounterInfoList);
esxVI_PerfQuerySpec_Free(&querySpec);
esxVI_PerfEntityMetricBase_Free(&perfEntityMetricBaseList);
+#endif
return result;
}
--
1.7.0.4
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list