7:45 a.m.
---
src/libvirt.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
1 files changed, 94 insertions(+), 6 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c
index 787908e..87391c6 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -1390,7 +1390,7 @@ int
virConnectRef(virConnectPtr conn)
{
if ((!VIR_IS_CONNECT(conn))) {
- virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__);
virDispatchError(NULL);
return -1;
}
@@ -4487,7 +4487,6 @@ virDomainMigratePrepareTunnel(virConnectPtr conn,
const char *dname,
unsigned long bandwidth,
const char *dom_xml)
-
{
VIR_DEBUG("conn=%p, stream=%p, flags=%lu, dname=%s, "
"bandwidth=%lu, dom_xml=%s", conn, st, flags,
@@ -4994,6 +4993,12 @@ virDomainGetSchedulerType(virDomainPtr domain, int *nparams)
virDispatchError(NULL);
return NULL;
}
+
+ if (nparams == NULL) {
+ virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
conn = domain->conn;
if (conn->driver->domainGetSchedulerType){
@@ -5040,6 +5045,12 @@ virDomainGetSchedulerParameters(virDomainPtr domain,
virDispatchError(NULL);
return -1;
}
+
+ if (params == NULL || nparams == NULL) {
+ virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
conn = domain->conn;
if (conn->driver->domainGetSchedulerParameters) {
@@ -5084,6 +5095,12 @@ virDomainSetSchedulerParameters(virDomainPtr domain,
virDispatchError(NULL);
return -1;
}
+
+ if (params == NULL) {
+ virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (domain->conn->flags & VIR_CONNECT_RO) {
virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
goto error;
@@ -5534,7 +5551,7 @@ virDomainGetBlockInfo(virDomainPtr domain, const char *path,
virDomainBlockInfoP
virDispatchError(NULL);
return -1;
}
- if (info == NULL) {
+ if (path == NULL || info == NULL) {
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
@@ -6440,6 +6457,12 @@ virDomainAttachDevice(virDomainPtr domain, const char *xml)
virDispatchError(NULL);
return -1;
}
+
+ if (xml == NULL) {
+ virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (domain->conn->flags & VIR_CONNECT_RO) {
virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
goto error;
@@ -6500,6 +6523,12 @@ virDomainAttachDeviceFlags(virDomainPtr domain,
virDispatchError(NULL);
return -1;
}
+
+ if (xml == NULL) {
+ virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (domain->conn->flags & VIR_CONNECT_RO) {
virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
goto error;
@@ -6545,6 +6574,12 @@ virDomainDetachDevice(virDomainPtr domain, const char *xml)
virDispatchError(NULL);
return -1;
}
+
+ if (xml == NULL) {
+ virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (domain->conn->flags & VIR_CONNECT_RO) {
virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
goto error;
@@ -6601,6 +6636,12 @@ virDomainDetachDeviceFlags(virDomainPtr domain,
virDispatchError(NULL);
return -1;
}
+
+ if (xml == NULL) {
+ virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (domain->conn->flags & VIR_CONNECT_RO) {
virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
goto error;
@@ -6661,6 +6702,12 @@ virDomainUpdateDeviceFlags(virDomainPtr domain,
virDispatchError(NULL);
return -1;
}
+
+ if (xml == NULL) {
+ virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (domain->conn->flags & VIR_CONNECT_RO) {
virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
goto error;
@@ -7321,7 +7368,7 @@ int
virNetworkRef(virNetworkPtr network)
{
if ((!VIR_IS_CONNECTED_NETWORK(network))) {
- virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ virLibConnError(VIR_ERR_INVALID_NETWORK, __FUNCTION__);
virDispatchError(NULL);
return -1;
}
@@ -8187,7 +8234,7 @@ int
virInterfaceRef(virInterfacePtr iface)
{
if ((!VIR_IS_CONNECTED_INTERFACE(iface))) {
- virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ virLibConnError(VIR_ERR_INVALID_INTERFACE, __FUNCTION__);
virDispatchError(NULL);
return -1;
}
@@ -8630,7 +8677,7 @@ virStoragePoolLookupByVolume(virStorageVolPtr vol)
virResetLastError();
if (!VIR_IS_CONNECTED_STORAGE_VOL(vol)) {
- virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__);
+ virLibConnError(VIR_ERR_INVALID_STORAGE_VOL, __FUNCTION__);
virDispatchError(NULL);
return NULL;
}
@@ -9702,6 +9749,11 @@ virStorageVolCreateXML(virStoragePoolPtr pool,
return NULL;
}
+ if (xmldesc == NULL) {
+ virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (pool->conn->flags & VIR_CONNECT_RO) {
virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
goto error;
@@ -9758,6 +9810,11 @@ virStorageVolCreateXMLFrom(virStoragePoolPtr pool,
goto error;
}
+ if (xmldesc == NULL) {
+ virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (pool->conn->flags & VIR_CONNECT_RO ||
clonevol->conn->flags & VIR_CONNECT_RO) {
virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
@@ -10508,6 +10565,11 @@ int virNodeDeviceListCaps(virNodeDevicePtr dev,
return -1;
}
+ if (names == NULL || maxnames < 0) {
+ virLibNodeDeviceError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (dev->conn->deviceMonitor &&
dev->conn->deviceMonitor->deviceListCaps) {
int ret;
ret = dev->conn->deviceMonitor->deviceListCaps (dev, names, maxnames);
@@ -11764,6 +11826,11 @@ int virStreamSend(virStreamPtr stream,
return -1;
}
+ if (data == NULL) {
+ virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (stream->driver &&
stream->driver->streamSend) {
int ret;
@@ -11859,6 +11926,11 @@ int virStreamRecv(virStreamPtr stream,
return -1;
}
+ if (data == NULL) {
+ virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (stream->driver &&
stream->driver->streamRecv) {
int ret;
@@ -11935,6 +12007,11 @@ int virStreamSendAll(virStreamPtr stream,
return -1;
}
+ if (handler == NULL) {
+ virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto cleanup;
+ }
+
if (stream->flags & VIR_STREAM_NONBLOCK) {
virLibConnError(VIR_ERR_OPERATION_INVALID,
_("data sources cannot be used for non-blocking
streams"));
@@ -12032,6 +12109,11 @@ int virStreamRecvAll(virStreamPtr stream,
return -1;
}
+ if (handler == NULL) {
+ virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto cleanup;
+ }
+
if (stream->flags & VIR_STREAM_NONBLOCK) {
virLibConnError(VIR_ERR_OPERATION_INVALID,
_("data sinks cannot be used for non-blocking
streams"));
@@ -13746,6 +13828,12 @@ virDomainSnapshotCreateXML(virDomainPtr domain,
}
conn = domain->conn;
+
+ if (xmlDesc == NULL) {
+ virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (conn->flags & VIR_CONNECT_RO) {
virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
goto error;
--
1.7.0.4
8:14 a.m.
On Tue, May 17, 2011 at 02:45:44PM +0200, Matthias Bolte wrote:
---
src/libvirt.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
1 files changed, 94 insertions(+), 6 deletions(-)
Damn, that's a lot of missing checks or not ideal reports
diff --git a/src/libvirt.c b/src/libvirt.c
index 787908e..87391c6 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -1390,7 +1390,7 @@ int
virConnectRef(virConnectPtr conn)
{
if ((!VIR_IS_CONNECT(conn))) {
- virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__);
virDispatchError(NULL);
return -1;
}
@@ -4487,7 +4487,6 @@ virDomainMigratePrepareTunnel(virConnectPtr conn,
const char *dname,
unsigned long bandwidth,
const char *dom_xml)
-
{
VIR_DEBUG("conn=%p, stream=%p, flags=%lu, dname=%s, "
"bandwidth=%lu, dom_xml=%s", conn, st, flags,
@@ -4994,6 +4993,12 @@ virDomainGetSchedulerType(virDomainPtr domain, int *nparams)
virDispatchError(NULL);
return NULL;
}
+
+ if (nparams == NULL) {
+ virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
conn = domain->conn;
if (conn->driver->domainGetSchedulerType){
@@ -5040,6 +5045,12 @@ virDomainGetSchedulerParameters(virDomainPtr domain,
virDispatchError(NULL);
return -1;
}
+
+ if (params == NULL || nparams == NULL) {
+ virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
I was just wondering here if params being NULL couldn't be used to
find out the correct value for nparams, but looking at least at the
qemu driver code we really expect the array there.
conn = domain->conn;
if (conn->driver->domainGetSchedulerParameters) {
@@ -5084,6 +5095,12 @@ virDomainSetSchedulerParameters(virDomainPtr domain,
virDispatchError(NULL);
return -1;
}
+
+ if (params == NULL) {
+ virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (domain->conn->flags & VIR_CONNECT_RO) {
virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
goto error;
@@ -5534,7 +5551,7 @@ virDomainGetBlockInfo(virDomainPtr domain, const char *path,
virDomainBlockInfoP
virDispatchError(NULL);
return -1;
}
- if (info == NULL) {
+ if (path == NULL || info == NULL) {
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
@@ -6440,6 +6457,12 @@ virDomainAttachDevice(virDomainPtr domain, const char *xml)
virDispatchError(NULL);
return -1;
}
+
+ if (xml == NULL) {
+ virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (domain->conn->flags & VIR_CONNECT_RO) {
virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
goto error;
@@ -6500,6 +6523,12 @@ virDomainAttachDeviceFlags(virDomainPtr domain,
virDispatchError(NULL);
return -1;
}
+
+ if (xml == NULL) {
+ virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (domain->conn->flags & VIR_CONNECT_RO) {
virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
goto error;
@@ -6545,6 +6574,12 @@ virDomainDetachDevice(virDomainPtr domain, const char *xml)
virDispatchError(NULL);
return -1;
}
+
+ if (xml == NULL) {
+ virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (domain->conn->flags & VIR_CONNECT_RO) {
virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
goto error;
@@ -6601,6 +6636,12 @@ virDomainDetachDeviceFlags(virDomainPtr domain,
virDispatchError(NULL);
return -1;
}
+
+ if (xml == NULL) {
+ virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (domain->conn->flags & VIR_CONNECT_RO) {
virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
goto error;
@@ -6661,6 +6702,12 @@ virDomainUpdateDeviceFlags(virDomainPtr domain,
virDispatchError(NULL);
return -1;
}
+
+ if (xml == NULL) {
+ virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (domain->conn->flags & VIR_CONNECT_RO) {
virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
goto error;
@@ -7321,7 +7368,7 @@ int
virNetworkRef(virNetworkPtr network)
{
if ((!VIR_IS_CONNECTED_NETWORK(network))) {
- virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ virLibConnError(VIR_ERR_INVALID_NETWORK, __FUNCTION__);
virDispatchError(NULL);
return -1;
}
@@ -8187,7 +8234,7 @@ int
virInterfaceRef(virInterfacePtr iface)
{
if ((!VIR_IS_CONNECTED_INTERFACE(iface))) {
- virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ virLibConnError(VIR_ERR_INVALID_INTERFACE, __FUNCTION__);
virDispatchError(NULL);
return -1;
}
@@ -8630,7 +8677,7 @@ virStoragePoolLookupByVolume(virStorageVolPtr vol)
virResetLastError();
if (!VIR_IS_CONNECTED_STORAGE_VOL(vol)) {
- virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__);
+ virLibConnError(VIR_ERR_INVALID_STORAGE_VOL, __FUNCTION__);
virDispatchError(NULL);
return NULL;
}
@@ -9702,6 +9749,11 @@ virStorageVolCreateXML(virStoragePoolPtr pool,
return NULL;
}
+ if (xmldesc == NULL) {
+ virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (pool->conn->flags & VIR_CONNECT_RO) {
virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
goto error;
@@ -9758,6 +9810,11 @@ virStorageVolCreateXMLFrom(virStoragePoolPtr pool,
goto error;
}
+ if (xmldesc == NULL) {
+ virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (pool->conn->flags & VIR_CONNECT_RO ||
clonevol->conn->flags & VIR_CONNECT_RO) {
virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
@@ -10508,6 +10565,11 @@ int virNodeDeviceListCaps(virNodeDevicePtr dev,
return -1;
}
+ if (names == NULL || maxnames < 0) {
+ virLibNodeDeviceError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (dev->conn->deviceMonitor &&
dev->conn->deviceMonitor->deviceListCaps) {
int ret;
ret = dev->conn->deviceMonitor->deviceListCaps (dev, names, maxnames);
@@ -11764,6 +11826,11 @@ int virStreamSend(virStreamPtr stream,
return -1;
}
+ if (data == NULL) {
+ virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (stream->driver &&
stream->driver->streamSend) {
int ret;
@@ -11859,6 +11926,11 @@ int virStreamRecv(virStreamPtr stream,
return -1;
}
+ if (data == NULL) {
+ virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (stream->driver &&
stream->driver->streamRecv) {
int ret;
@@ -11935,6 +12007,11 @@ int virStreamSendAll(virStreamPtr stream,
return -1;
}
+ if (handler == NULL) {
+ virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto cleanup;
+ }
+
if (stream->flags & VIR_STREAM_NONBLOCK) {
virLibConnError(VIR_ERR_OPERATION_INVALID,
_("data sources cannot be used for non-blocking
streams"));
@@ -12032,6 +12109,11 @@ int virStreamRecvAll(virStreamPtr stream,
return -1;
}
+ if (handler == NULL) {
+ virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto cleanup;
+ }
+
if (stream->flags & VIR_STREAM_NONBLOCK) {
virLibConnError(VIR_ERR_OPERATION_INVALID,
_("data sinks cannot be used for non-blocking
streams"));
@@ -13746,6 +13828,12 @@ virDomainSnapshotCreateXML(virDomainPtr domain,
}
conn = domain->conn;
+
+ if (xmlDesc == NULL) {
+ virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
+ goto error;
+ }
+
if (conn->flags & VIR_CONNECT_RO) {
virLibConnError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
goto error;
ACK, seems all those check are better handled directly in the front end
function
thanks !
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
10:46 a.m.
On 05/17/2011 07:14 AM, Daniel Veillard wrote:
On Tue, May 17, 2011 at 02:45:44PM +0200, Matthias Bolte wrote:
> ---
> src/libvirt.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 files changed, 94 insertions(+), 6 deletions(-)
> @@ -5040,6 +5045,12 @@ virDomainGetSchedulerParameters(virDomainPtr domain,
> virDispatchError(NULL);
> return -1;
> }
> +
> + if (params == NULL || nparams == NULL) {
> + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> + goto error;
> + }
I was just wondering here if params being NULL couldn't be used to
find out the correct value for nparams, but looking at least at the
qemu driver code we really expect the array there.
Let's fix that as a separate patch.
> conn = domain->conn;
>
> if (conn->driver->domainGetSchedulerParameters) {
> @@ -5084,6 +5095,12 @@ virDomainSetSchedulerParameters(virDomainPtr domain,
> virDispatchError(NULL);
> return -1;
> }
> +
> + if (params == NULL) {
> + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> + goto error;
> + }
Hmm, here we document that nparams can be <= the value returned by
virDomainGetSchedulerType; which means it can be 0, which means that
params can be NULL. I think we should change this to allow NULL,0 in
input as a way of querying the proper nparams size on output.
This affects Hu's patch series, where we are adding
virDomainSetSchedulerParametersFlags, which should have the same semantics.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:12 p.m.
2011/5/17 Eric Blake <eblake(a)redhat.com>:
On 05/17/2011 07:14 AM, Daniel Veillard wrote:
> On Tue, May 17, 2011 at 02:45:44PM +0200, Matthias Bolte wrote:
>> ---
>> src/libvirt.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
>> 1 files changed, 94 insertions(+), 6 deletions(-)
>
>> @@ -5040,6 +5045,12 @@ virDomainGetSchedulerParameters(virDomainPtr domain,
>> virDispatchError(NULL);
>> return -1;
>> }
>> +
>> + if (params == NULL || nparams == NULL) {
>> + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
>> + goto error;
>> + }
>
> I was just wondering here if params being NULL couldn't be used to
> find out the correct value for nparams, but looking at least at the
> qemu driver code we really expect the array there.
Let's fix that as a separate patch.
The actual semantic for (n)params is not completely defined, is it?
This just matches what (most of?) the driver currently do.
>
>> conn = domain->conn;
>>
>> if (conn->driver->domainGetSchedulerParameters) {
>> @@ -5084,6 +5095,12 @@ virDomainSetSchedulerParameters(virDomainPtr domain,
>> virDispatchError(NULL);
>> return -1;
>> }
>> +
>> + if (params == NULL) {
>> + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
>> + goto error;
>> + }
Hmm, here we document that nparams can be <= the value returned by
virDomainGetSchedulerType; which means it can be 0, which means that
params can be NULL. I think we should change this to allow NULL,0 in
input as a way of querying the proper nparams size on output.
Why would you add a second way to query nparams?
This affects Hu's patch series, where we are adding
virDomainSetSchedulerParametersFlags, which should have the same semantics.
Well, it's documented that nparams can be less than what
virDomainGetSchedulerType returned, but that's not how is implemented
in the drivers. The drivers fail if you pass them nparams <
virDomainGetSchedulerType. Also it doesn't make that mush sense to
query less than all parameters as you cannot request for a specific
subset explicitly.
Matthias
1:56 p.m.
On 05/17/2011 12:12 PM, Matthias Bolte wrote:
The actual semantic for (n)params is not completely defined, is it?
This just matches what (most of?) the driver currently do.
> Hmm, here we document that nparams can be <= the value returned by
> virDomainGetSchedulerType; which means it can be 0, which means that
> params can be NULL. I think we should change this to allow NULL,0 in
> input as a way of querying the proper nparams size on output.
Why would you add a second way to query nparams?
I guess the alternative is to tighten up the documentation to
specifically state that nparams must be the number of parameters (and
not a subset) managed by the device for Get; Set can still do subsets
though.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:06 p.m.
On 05/17/2011 12:56 PM, Eric Blake wrote:
On 05/17/2011 12:12 PM, Matthias Bolte wrote:
>
> The actual semantic for (n)params is not completely defined, is it?
> This just matches what (most of?) the driver currently do.
>
>> Hmm, here we document that nparams can be <= the value returned by
>> virDomainGetSchedulerType; which means it can be 0, which means that
>> params can be NULL. I think we should change this to allow NULL,0 in
>> input as a way of querying the proper nparams size on output.
>
> Why would you add a second way to query nparams?
I guess the alternative is to tighten up the documentation to
specifically state that nparams must be the number of parameters (and
not a subset) managed by the device for Get; Set can still do subsets
though.
I went with the documentation alternative in this patch [1], although I
didn't add the explicit NULL checking to the new API functions (neither
in Hu's virDomainSetSchedulerParametersFlags pushed today, nor my
proposed virDomainGetSchedulerParametersFlags in [1]), so depending on
which one gets pushed first, the other patch should probably add some
NULL checking.
[1] https://www.redhat.com/archives/libvir-list/2011-May/msg01146.html
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
8:55 p.m.
On Tue, May 17, 2011 at 09:46:34AM -0600, Eric Blake wrote:
On 05/17/2011 07:14 AM, Daniel Veillard wrote:
> On Tue, May 17, 2011 at 02:45:44PM +0200, Matthias Bolte wrote:
>> ---
>> src/libvirt.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
>> 1 files changed, 94 insertions(+), 6 deletions(-)
>
>> @@ -5040,6 +5045,12 @@ virDomainGetSchedulerParameters(virDomainPtr domain,
>> virDispatchError(NULL);
>> return -1;
>> }
>> +
>> + if (params == NULL || nparams == NULL) {
>> + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
>> + goto error;
>> + }
>
> I was just wondering here if params being NULL couldn't be used to
> find out the correct value for nparams, but looking at least at the
> qemu driver code we really expect the array there.
Let's fix that as a separate patch.
>
>> conn = domain->conn;
>>
>> if (conn->driver->domainGetSchedulerParameters) {
>> @@ -5084,6 +5095,12 @@ virDomainSetSchedulerParameters(virDomainPtr domain,
>> virDispatchError(NULL);
>> return -1;
>> }
>> +
>> + if (params == NULL) {
>> + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
>> + goto error;
>> + }
Hmm, here we document that nparams can be <= the value returned by
virDomainGetSchedulerType; which means it can be 0, which means that
params can be NULL. I think we should change this to allow NULL,0 in
input as a way of querying the proper nparams size on output.
This affects Hu's patch series, where we are adding
virDomainSetSchedulerParametersFlags, which should have the same semantics.
I was somehow remembering that we had let open the possibility to not
provide the full array. Looking at the function comment which is the API
contract there was nothing about it, so I looked at qemu backend and
it fails if nparam != 1 , so at least that driver implemented the strict
API.
We need to decide either way, possibly by looking first at all API
entry points using that mechanism array pointer + array size, and see
if we are likely to break any app if we make it strict. Or we could
decide to allow 0 and NULL and in that case we need to document
explicitely the semantic.
However for virDomainGetSchedulerParameters() it's relatively clear
(well that could possibly be improved a bit) that the application must
call virDomainGetSchedulerType() first to get nparams value, then
allocate accordingly), so I would side on imposing the strict behaviour
but make it a bit clearer.
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
3:20 a.m.
2011/5/18 Daniel Veillard <veillard(a)redhat.com>:
On Tue, May 17, 2011 at 09:46:34AM -0600, Eric Blake wrote:
> On 05/17/2011 07:14 AM, Daniel Veillard wrote:
> > On Tue, May 17, 2011 at 02:45:44PM +0200, Matthias Bolte wrote:
> >> ---
> >> src/libvirt.c | 100
+++++++++++++++++++++++++++++++++++++++++++++++++++++---
> >> 1 files changed, 94 insertions(+), 6 deletions(-)
> >
> >> @@ -5040,6 +5045,12 @@ virDomainGetSchedulerParameters(virDomainPtr domain,
> >> virDispatchError(NULL);
> >> return -1;
> >> }
> >> +
> >> + if (params == NULL || nparams == NULL) {
> >> + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> >> + goto error;
> >> + }
> >
> > I was just wondering here if params being NULL couldn't be used to
> > find out the correct value for nparams, but looking at least at the
> > qemu driver code we really expect the array there.
>
> Let's fix that as a separate patch.
>
> >
> >> conn = domain->conn;
> >>
> >> if (conn->driver->domainGetSchedulerParameters) {
> >> @@ -5084,6 +5095,12 @@ virDomainSetSchedulerParameters(virDomainPtr domain,
> >> virDispatchError(NULL);
> >> return -1;
> >> }
> >> +
> >> + if (params == NULL) {
> >> + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> >> + goto error;
> >> + }
>
> Hmm, here we document that nparams can be <= the value returned by
> virDomainGetSchedulerType; which means it can be 0, which means that
> params can be NULL. I think we should change this to allow NULL,0 in
> input as a way of querying the proper nparams size on output.
>
> This affects Hu's patch series, where we are adding
> virDomainSetSchedulerParametersFlags, which should have the same semantics.
I was somehow remembering that we had let open the possibility to not
provide the full array. Looking at the function comment which is the API
contract there was nothing about it, so I looked at qemu backend and
it fails if nparam != 1 , so at least that driver implemented the strict
API.
We need to decide either way, possibly by looking first at all API
entry points using that mechanism array pointer + array size, and see
if we are likely to break any app if we make it strict. Or we could
decide to allow 0 and NULL and in that case we need to document
explicitely the semantic.
However for virDomainGetSchedulerParameters() it's relatively clear
(well that could possibly be improved a bit) that the application must
call virDomainGetSchedulerType() first to get nparams value, then
allocate accordingly), so I would side on imposing the strict behaviour
but make it a bit clearer.
Daniel
I'm working on a series to clean this up and go for the stricter
semantic of the Get*Parameters functions.
Matthias
11:30 a.m.
2011/5/18 Matthias Bolte <matthias.bolte(a)googlemail.com>:
2011/5/18 Daniel Veillard <veillard(a)redhat.com>:
> On Tue, May 17, 2011 at 09:46:34AM -0600, Eric Blake wrote:
>> On 05/17/2011 07:14 AM, Daniel Veillard wrote:
>> > On Tue, May 17, 2011 at 02:45:44PM +0200, Matthias Bolte wrote:
>> >> ---
>> >> src/libvirt.c | 100
+++++++++++++++++++++++++++++++++++++++++++++++++++++---
>> >> 1 files changed, 94 insertions(+), 6 deletions(-)
>> >
>> >> @@ -5040,6 +5045,12 @@ virDomainGetSchedulerParameters(virDomainPtr
domain,
>> >> virDispatchError(NULL);
>> >> return -1;
>> >> }
>> >> +
>> >> + if (params == NULL || nparams == NULL) {
>> >> + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
>> >> + goto error;
>> >> + }
>> >
>> > I was just wondering here if params being NULL couldn't be used to
>> > find out the correct value for nparams, but looking at least at the
>> > qemu driver code we really expect the array there.
>>
>> Let's fix that as a separate patch.
>>
>> >
>> >> conn = domain->conn;
>> >>
>> >> if (conn->driver->domainGetSchedulerParameters) {
>> >> @@ -5084,6 +5095,12 @@ virDomainSetSchedulerParameters(virDomainPtr
domain,
>> >> virDispatchError(NULL);
>> >> return -1;
>> >> }
>> >> +
>> >> + if (params == NULL) {
>> >> + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
>> >> + goto error;
>> >> + }
>>
>> Hmm, here we document that nparams can be <= the value returned by
>> virDomainGetSchedulerType; which means it can be 0, which means that
>> params can be NULL. I think we should change this to allow NULL,0 in
>> input as a way of querying the proper nparams size on output.
>>
>> This affects Hu's patch series, where we are adding
>> virDomainSetSchedulerParametersFlags, which should have the same semantics.
>
> I was somehow remembering that we had let open the possibility to not
> provide the full array. Looking at the function comment which is the API
> contract there was nothing about it, so I looked at qemu backend and
> it fails if nparam != 1 , so at least that driver implemented the strict
> API.
> We need to decide either way, possibly by looking first at all API
> entry points using that mechanism array pointer + array size, and see
> if we are likely to break any app if we make it strict. Or we could
> decide to allow 0 and NULL and in that case we need to document
> explicitely the semantic.
> However for virDomainGetSchedulerParameters() it's relatively clear
> (well that could possibly be improved a bit) that the application must
> call virDomainGetSchedulerType() first to get nparams value, then
> allocate accordingly), so I would side on imposing the strict behaviour
> but make it a bit clearer.
>
> Daniel
I'm working on a series to clean this up and go for the stricter
semantic of the Get*Parameters functions.
Matthias
The initial patch has been superseded by this series:
https://www.redhat.com/archives/libvir-list/2011-May/msg01194.html
Matthias
4939
days inactive
4940
days old
8 comments
3 participants
participants (3)
-
Daniel Veillard -
Eric Blake -
Matthias Bolte