[libvirt] [PATCH] Improve invalid argument checks for the public API
--- 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
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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2011/5/17 Eric Blake <eblake@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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
2011/5/18 Daniel Veillard <veillard@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
2011/5/18 Matthias Bolte <matthias.bolte@googlemail.com>:
2011/5/18 Daniel Veillard <veillard@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
participants (3)
-
Daniel Veillard -
Eric Blake -
Matthias Bolte