Daniel P. Berrange wrote:
> On Tue, Jan 26, 2010 at 08:24:25PM +0100, Jim Meyering wrote:
>> When "domain" is NULL, don't deref NULL. Instead, just return -1,
>> as in many other functions in this file, and as this function did
>> up until a month ago.
>>
>> An alternative (taken 3 times in this file) is to do this:
>>
>> virXenErrorFunc (NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__,
>> "domain or conn is NULL", 0);
>> return -1;
>>
>> I could go either way.
>>
>>
>> >From 177556167775b806a29bcb1af7ba4294d1909912 Mon Sep 17 00:00:00 2001
>> From: Jim Meyering <meyering@xxxxxxxxxx>
>> Date: Tue, 26 Jan 2010 20:17:07 +0100
>> Subject: [PATCH] xen_hypervisor.c: avoid NULL deref for NULL domain argument
>>
>> * src/xen/xen_hypervisor.c (xenHypervisorGetVcpus): Don't attempt
>> to diagnose an unlikely NULL-domain or NULL-domain->conn error.
>> ---
>> src/xen/xen_hypervisor.c | 7 ++-----
>> 1 files changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c
>> index 6d8accc..0257be2 100644
>> --- a/src/xen/xen_hypervisor.c
>> +++ b/src/xen/xen_hypervisor.c
>> @@ -1,7 +1,7 @@
>> /*
>> * xen_internal.c: direct access to Xen hypervisor level
>> *
>> - * Copyright (C) 2005, 2006, 2007, 2008, 2009 Red Hat, Inc.
>> + * Copyright (C) 2005-2010 Red Hat, Inc.
>> *
>> * See COPYING.LIB for the License of this software
>> *
>> @@ -3475,11 +3475,8 @@ xenHypervisorGetVcpus(virDomainPtr domain, virVcpuInfoPtr info, int maxinfo,
>> virVcpuInfoPtr ipt;
>> int nbinfo, i;
>>
>> - if (domain == NULL || domain->conn == NULL) {
>> - virXenErrorFunc (domain->conn, VIR_ERR_INVALID_ARG, __FUNCTION__,
>> - "invalid argument", 0);
>> + if (domain == NULL || domain->conn == NULL)
>> return -1;
>> - }
>
> I'd rather we just got rid of that check completely - its a left
> over from a time when Xen was the only driver & these entry points
> needed to do full checking. Now all mandatory parameters are checked
> in the previous libvirt.c layer.
Here's an additional patch, to eliminate all of the "domain == NULL"
tests. I want to keep this global "clean-up" patch separate from
the above bug-fixing one.
I'm less confident about removing the daomin->conn tests,
and would be inclined to leave them as-is, or use an assert, if you
want to remove them. If we also remove the daomin->conn == NULL tests,
an added "assert" is the best way to keep clang/coverity from
complaining about a possible NULL-dereference.
>From 9e6f7ca7a0dfa6b220e598d04ca40d33e67feb22 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@xxxxxxxxxx>
Date: Wed, 27 Jan 2010 13:34:03 +0100
Subject: [PATCH] xen_hypervisor.c: remove all "domain == NULL" tests, ...
* src/xen/xen_hypervisor.c: Remove all "domain == NULL" tests.
* src/xen/xen_hypervisor.h: Instead, use ATTRIBUTE_NONNULL to
mark each "domain" parameter as "known always to be non-NULL".
---
src/xen/xen_hypervisor.c | 28 ++++++++++++++--------------
src/xen/xen_hypervisor.h | 44 +++++++++++++++++++++++++++++---------------
2 files changed, 43 insertions(+), 29 deletions(-)
diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c
index 0257be2..994f5ef 100644
--- a/src/xen/xen_hypervisor.c
+++ b/src/xen/xen_hypervisor.c
@@ -1130,7 +1130,7 @@ xenHypervisorGetSchedulerType(virDomainPtr domain, int *nparams)
char *schedulertype = NULL;
xenUnifiedPrivatePtr priv;
- if ((domain == NULL) || (domain->conn == NULL)) {
+ if (domain->conn == NULL) {
virXenErrorFunc(NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__,
"domain or conn is NULL", 0);
return NULL;
@@ -1214,7 +1214,7 @@ xenHypervisorGetSchedulerParameters(virDomainPtr domain,
{
xenUnifiedPrivatePtr priv;
- if ((domain == NULL) || (domain->conn == NULL)) {
+ if (domain->conn == NULL) {
virXenErrorFunc(NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__,
"domain or conn is NULL", 0);
return -1;
@@ -1317,7 +1317,7 @@ xenHypervisorSetSchedulerParameters(virDomainPtr domain,
xenUnifiedPrivatePtr priv;
char buf[256];
- if ((domain == NULL) || (domain->conn == NULL)) {
+ if (domain->conn == NULL) {
virXenErrorFunc (NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__,
"domain or conn is NULL", 0);
return -1;
@@ -3062,12 +3062,12 @@ xenHypervisorGetDomMaxMemory(virConnectPtr conn, int id)
*
* Returns the memory size in kilobytes or 0 in case of error.
*/
-static unsigned long
+static unsigned long ATTRIBUTE_NONNULL (1)
xenHypervisorGetMaxMemory(virDomainPtr domain)
{
xenUnifiedPrivatePtr priv;
- if ((domain == NULL) || (domain->conn == NULL))
+ if (domain->conn == NULL)
return 0;
priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
@@ -3176,7 +3176,7 @@ xenHypervisorGetDomainInfo(virDomainPtr domain, virDomainInfoPtr info)
{
xenUnifiedPrivatePtr priv;
- if ((domain == NULL) || (domain->conn == NULL))
+ if (domain->conn == NULL)
return -1;
priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
@@ -3284,7 +3284,7 @@ xenHypervisorPauseDomain(virDomainPtr domain)
int ret;
xenUnifiedPrivatePtr priv;
- if ((domain == NULL) || (domain->conn == NULL))
+ if (domain->conn == NULL)
return -1;
priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
@@ -3311,7 +3311,7 @@ xenHypervisorResumeDomain(virDomainPtr domain)
int ret;
xenUnifiedPrivatePtr priv;
- if ((domain == NULL) || (domain->conn == NULL))
+ if (domain->conn == NULL)
return -1;
priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
@@ -3338,7 +3338,7 @@ xenHypervisorDestroyDomain(virDomainPtr domain)
int ret;
xenUnifiedPrivatePtr priv;
- if (domain == NULL || domain->conn == NULL)
+ if (domain->conn == NULL)
return -1;
priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
@@ -3366,7 +3366,7 @@ xenHypervisorSetMaxMemory(virDomainPtr domain, unsigned long memory)
int ret;
xenUnifiedPrivatePtr priv;
- if (domain == NULL || domain->conn == NULL)
+ if (domain->conn == NULL)
return -1;
priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
@@ -3397,7 +3397,7 @@ xenHypervisorSetVcpus(virDomainPtr domain, unsigned int nvcpus)
int ret;
xenUnifiedPrivatePtr priv;
- if (domain == NULL || domain->conn == NULL)
+ if (domain->conn == NULL)
return -1;
priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
@@ -3429,7 +3429,7 @@ xenHypervisorPinVcpu(virDomainPtr domain, unsigned int vcpu,
int ret;
xenUnifiedPrivatePtr priv;
- if (domain == NULL || domain->conn == NULL)
+ if (domain->conn == NULL)
return -1;
priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
@@ -3475,7 +3475,7 @@ xenHypervisorGetVcpus(virDomainPtr domain, virVcpuInfoPtr info, int maxinfo,
virVcpuInfoPtr ipt;
int nbinfo, i;
- if (domain == NULL || domain->conn == NULL)
+ if (domain->conn == NULL)
return -1;
priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
@@ -3548,7 +3548,7 @@ xenHypervisorGetVcpuMax(virDomainPtr domain)
int maxcpu;
xenUnifiedPrivatePtr priv;
- if (domain == NULL || domain->conn == NULL)
+ if (domain->conn == NULL)
return -1;
priv = (xenUnifiedPrivatePtr) domain->conn->privateData;
diff --git a/src/xen/xen_hypervisor.h b/src/xen/xen_hypervisor.h
index 5971a90..4504733 100644
--- a/src/xen/xen_hypervisor.h
+++ b/src/xen/xen_hypervisor.h
@@ -1,7 +1,7 @@
/*
* xen_internal.h: internal API for direct access to Xen hypervisor level
*
- * Copyright (C) 2005 Red Hat, Inc.
+ * Copyright (C) 2005, 2010 Red Hat, Inc.
*
* See COPYING.LIB for the License of this software
*
@@ -58,48 +58,62 @@ int xenHypervisorListDomains (virConnectPtr conn,
int maxids);
int xenHypervisorGetMaxVcpus (virConnectPtr conn,
const char *type);
-int xenHypervisorDestroyDomain (virDomainPtr domain);
-int xenHypervisorResumeDomain (virDomainPtr domain);
-int xenHypervisorPauseDomain (virDomainPtr domain);
+int xenHypervisorDestroyDomain (virDomainPtr domain)
+ ATTRIBUTE_NONNULL (1);
+int xenHypervisorResumeDomain (virDomainPtr domain)
+ ATTRIBUTE_NONNULL (1);
+int xenHypervisorPauseDomain (virDomainPtr domain)
+ ATTRIBUTE_NONNULL (1);
int xenHypervisorGetDomainInfo (virDomainPtr domain,
- virDomainInfoPtr info);
+ virDomainInfoPtr info)
+ ATTRIBUTE_NONNULL (1);
int xenHypervisorGetDomInfo (virConnectPtr conn,
int id,
virDomainInfoPtr info);
int xenHypervisorSetMaxMemory (virDomainPtr domain,
- unsigned long memory);
+ unsigned long memory)
+ ATTRIBUTE_NONNULL (1);
int xenHypervisorCheckID (virConnectPtr conn,
int id);
int xenHypervisorSetVcpus (virDomainPtr domain,
- unsigned int nvcpus);
+ unsigned int nvcpus)
+ ATTRIBUTE_NONNULL (1);
int xenHypervisorPinVcpu (virDomainPtr domain,
unsigned int vcpu,
unsigned char *cpumap,
- int maplen);
+ int maplen)
+ ATTRIBUTE_NONNULL (1);
int xenHypervisorGetVcpus (virDomainPtr domain,
virVcpuInfoPtr info,
int maxinfo,
unsigned char *cpumaps,
- int maplen);
-int xenHypervisorGetVcpuMax (virDomainPtr domain);
+ int maplen)
+ ATTRIBUTE_NONNULL (1);
+int xenHypervisorGetVcpuMax (virDomainPtr domain)
+ ATTRIBUTE_NONNULL (1);
char * xenHypervisorGetSchedulerType (virDomainPtr domain,
- int *nparams);
+ int *nparams)
+ ATTRIBUTE_NONNULL (1);
int xenHypervisorGetSchedulerParameters(virDomainPtr domain,
virSchedParameterPtr params,
- int *nparams);
+ int *nparams)
+ ATTRIBUTE_NONNULL (1);
int xenHypervisorSetSchedulerParameters(virDomainPtr domain,
virSchedParameterPtr params,
- int nparams);
+ int nparams)
+ ATTRIBUTE_NONNULL (1);
int xenHypervisorDomainBlockStats (virDomainPtr domain,
const char *path,
- struct _virDomainBlockStats *stats);
+ struct _virDomainBlockStats *stats)
+ ATTRIBUTE_NONNULL (1);
int xenHypervisorDomainInterfaceStats (virDomainPtr domain,
const char *path,
- struct _virDomainInterfaceStats *stats);
+ struct _virDomainInterfaceStats *stats)
+ ATTRIBUTE_NONNULL (1);
int xenHypervisorNodeGetCellsFreeMemory(virConnectPtr conn,
unsigned long long *freeMems,
--
1.7.0.rc0.140.gfbe7
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list