[libvirt] [PATCH 0/6] Clarify semantic of virDomain(G|S)et*Parameters functions
This series was inspired by the discusion about invalid argument checking in the public API and the unclear semantic of this functions. https://www.redhat.com/archives/libvir-list/2011-May/msg01105.html Clarify that the getter functions don't allow to obtain a subset of the parameters. You can only get all at once. Clarify that the setter functions can set a subset of the parameters. Matthias
Some drivers assumed it can be NULL (e.g. qemu and lxc) and check it before assigning to it, other drivers assumed it must be non-NULL (e.g. test and esx) and just assigned to it. Unify this to nparams being optional and document it. --- src/esx/esx_driver.c | 4 +++- src/libvirt.c | 5 +++-- src/libxl/libxl_driver.c | 6 ++++-- src/test/test_driver.c | 4 +++- src/xen/xend_internal.c | 9 +++++---- src/xenapi/xenapi_driver.c | 4 +++- 6 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index bb9d60a..7fe5446 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -3513,7 +3513,9 @@ esxDomainGetSchedulerType(virDomainPtr domain ATTRIBUTE_UNUSED, int *nparams) return NULL; } - *nparams = 3; /* reservation, limit, shares */ + if (nparams != NULL) { + *nparams = 3; /* reservation, limit, shares */ + } return type; } diff --git a/src/libvirt.c b/src/libvirt.c index 56b1257..45bc9b0 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -4973,9 +4973,10 @@ error: /** * virDomainGetSchedulerType: * @domain: pointer to domain object - * @nparams: number of scheduler parameters(return value) + * @nparams: pointer to number of scheduler parameters, can be NULL + * (return value) * - * Get the scheduler type. + * Get the scheduler type and the number of scheduler parameters. * * Returns NULL in case of error. The caller must free the returned string. */ diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 5cc9362..6b2d806 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2396,14 +2396,16 @@ libxlDomainGetSchedulerType(virDomainPtr dom, int *nparams) goto cleanup; } - *nparams = 0; + if (nparams) + *nparams = 0; switch(sched_id) { case XEN_SCHEDULER_SEDF: ret = strdup("sedf"); break; case XEN_SCHEDULER_CREDIT: ret = strdup("credit"); - *nparams = XEN_SCHED_CREDIT_NPARAM; + if (nparams) + *nparams = XEN_SCHED_CREDIT_NPARAM; break; case XEN_SCHEDULER_CREDIT2: ret = strdup("credit2"); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 658bcee..d9e9cb9 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2617,7 +2617,9 @@ static char *testDomainGetSchedulerType(virDomainPtr domain ATTRIBUTE_UNUSED, { char *type = NULL; - *nparams = 1; + if (nparams) + *nparams = 1; + type = strdup("fair"); if (!type) virReportOOMError(); diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index dfa0342..359d5f4 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -3531,8 +3531,7 @@ xenDaemonGetSchedulerType(virDomainPtr domain, int *nparams) const char *ret = NULL; char *schedulertype = NULL; - if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL) - || (nparams == NULL)) { + if (domain->conn == NULL || domain->name == NULL) { virXendError(VIR_ERR_INVALID_ARG, __FUNCTION__); return NULL; } @@ -3562,14 +3561,16 @@ xenDaemonGetSchedulerType(virDomainPtr domain, int *nparams) virReportOOMError(); goto error; } - *nparams = XEN_SCHED_CRED_NPARAM; + if (nparams) + *nparams = XEN_SCHED_CRED_NPARAM; } else if (STREQ (ret, "sedf")) { schedulertype = strdup("sedf"); if (schedulertype == NULL){ virReportOOMError(); goto error; } - *nparams = XEN_SCHED_SEDF_NPARAM; + if (nparams) + *nparams = XEN_SCHED_SEDF_NPARAM; } else { virXendError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unknown scheduler")); goto error; diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 13b9161..6f64208 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -1767,7 +1767,9 @@ static char * xenapiDomainGetSchedulerType (virDomainPtr dom ATTRIBUTE_UNUSED, int *nparams) { char *result = NULL; - *nparams = 0; + + if (nparams) + *nparams = 0; if (!(result = strdup("credit"))) virReportOOMError(); return result; -- 1.7.0.4
params and nparams are essential and cannot be NULL. Check this in libvirt.c and remove redundant checks from the drivers (e.g. xend). Instead of enforcing that nparams must point to exact same value as returned by virDomainGetSchedulerType relax this to a lower bound check. This is what some drivers (e.g. xen hypervisor and esx) already did. Other drivers (e.g. xend) didn't check nparams at all and assumed that there is enough space in params. Unify the behavior in all drivers to a lower bound check and update nparams to the number of valid values in params on success. --- src/libvirt.c | 17 ++++++++++++----- src/libxl/libxl_driver.c | 3 ++- src/lxc/lxc_driver.c | 3 ++- src/qemu/qemu_driver.c | 3 ++- src/test/test_driver.c | 6 ++++-- src/xen/xen_driver.h | 3 +++ src/xen/xen_hypervisor.c | 23 ++++++++++++++++------- src/xen/xend_internal.c | 17 +++++++++++++---- 8 files changed, 54 insertions(+), 21 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 45bc9b0..e8b5b80 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -5015,14 +5015,15 @@ error: /** * virDomainGetSchedulerParameters: * @domain: pointer to domain object - * @params: pointer to scheduler parameter object + * @params: pointer to scheduler parameter objects * (return value) - * @nparams: pointer to number of scheduler parameter - * (this value should be same than the returned value + * @nparams: pointer to number of scheduler parameter objects + * (this value must be at least as large as the returned value * nparams of virDomainGetSchedulerType) * - * Get the scheduler parameters, the @params array will be filled with the - * values. + * Get all scheduler parameters, the @params array will be filled with the + * values and @nparams will be updated to the number of valid elements in + * @params. * * Returns -1 in case of error, 0 in case of success. */ @@ -5041,6 +5042,12 @@ virDomainGetSchedulerParameters(virDomainPtr domain, virDispatchError(NULL); return -1; } + + if (params == NULL || nparams == NULL || *nparams < 0) { + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + conn = domain->conn; if (conn->driver->domainGetSchedulerParameters) { diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 6b2d806..b85c743 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2466,7 +2466,7 @@ libxlDomainGetSchedulerParameters(virDomainPtr dom, virSchedParameterPtr params, goto cleanup; } - if (*nparams != XEN_SCHED_CREDIT_NPARAM) { + if (*nparams < XEN_SCHED_CREDIT_NPARAM) { libxlError(VIR_ERR_INVALID_ARG, "%s", _("Invalid parameter count")); goto cleanup; } @@ -2494,6 +2494,7 @@ libxlDomainGetSchedulerParameters(virDomainPtr dom, virSchedParameterPtr params, goto cleanup; } + *nparams = XEN_SCHED_CREDIT_NPARAM; ret = 0; cleanup: diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 2bb592d..a65299b 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2234,7 +2234,7 @@ static int lxcGetSchedulerParameters(virDomainPtr domain, if (driver->cgroup == NULL) return -1; - if ((*nparams) != 1) { + if (*nparams < 1) { lxcError(VIR_ERR_INVALID_ARG, "%s", _("Invalid parameter count")); return -1; @@ -2264,6 +2264,7 @@ static int lxcGetSchedulerParameters(virDomainPtr domain, } params[0].type = VIR_DOMAIN_SCHED_FIELD_ULLONG; + *nparams = 1; ret = 0; cleanup: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fdb3b30..9a7286d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5181,7 +5181,7 @@ static int qemuGetSchedulerParameters(virDomainPtr dom, goto cleanup; } - if ((*nparams) != 1) { + if (*nparams < 1) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid parameter count")); goto cleanup; @@ -5221,6 +5221,7 @@ out: goto cleanup; } + *nparams = 1; ret = 0; cleanup: diff --git a/src/test/test_driver.c b/src/test/test_driver.c index d9e9cb9..4897081 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2645,8 +2645,8 @@ static int testDomainGetSchedulerParams(virDomainPtr domain, goto cleanup; } - if (*nparams != 1) { - testError(VIR_ERR_INVALID_ARG, "nparams"); + if (*nparams < 1) { + testError(VIR_ERR_INVALID_ARG, "%s", _("Invalid parameter count")); goto cleanup; } strcpy(params[0].field, "weight"); @@ -2654,6 +2654,8 @@ static int testDomainGetSchedulerParams(virDomainPtr domain, /* XXX */ /*params[0].value.ui = privdom->weight;*/ params[0].value.ui = 50; + + *nparams = 1; ret = 0; cleanup: diff --git a/src/xen/xen_driver.h b/src/xen/xen_driver.h index 8fb5832..20eca6e 100644 --- a/src/xen/xen_driver.h +++ b/src/xen/xen_driver.h @@ -60,6 +60,9 @@ extern int xenRegister (void); # define XEND_DOMAINS_DIR "/var/lib/xend/domains" +# define XEN_SCHED_SEDF_NPARAM 6 +# define XEN_SCHED_CRED_NPARAM 2 + /* _xenUnifiedDriver: * * Entry points into the underlying Xen drivers. This structure diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index 3ec6e2b..8d579bc 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -1207,14 +1207,14 @@ xenHypervisorGetSchedulerType(virDomainPtr domain, int *nparams) if (schedulertype == NULL) virReportOOMError(); if (nparams) - *nparams = 6; + *nparams = XEN_SCHED_SEDF_NPARAM; break; case XEN_SCHEDULER_CREDIT: schedulertype = strdup("credit"); if (schedulertype == NULL) virReportOOMError(); if (nparams) - *nparams = 2; + *nparams = XEN_SCHED_CRED_NPARAM; break; default: break; @@ -1232,8 +1232,8 @@ static const char *str_cap = "cap"; * @domain: pointer to the Xen Hypervisor block * @params: pointer to scheduler parameters. * This memory area should be allocated before calling. - * @nparams:this parameter should be same as - * a given number of scheduler parameters. + * @nparams: this parameter must be at least as large as + * the given number of scheduler parameters. * from xenHypervisorGetSchedulerType(). * * Do a low level hypercall to get scheduler parameters @@ -1288,12 +1288,21 @@ xenHypervisorGetSchedulerParameters(virDomainPtr domain, switch (op_sys.u.getschedulerid.sched_id){ case XEN_SCHEDULER_SEDF: + if (*nparams < XEN_SCHED_SEDF_NPARAM) { + virXenError(VIR_ERR_INVALID_ARG, + "%s", _("Invalid parameter count")); + return -1; + } + /* TODO: Implement for Xen/SEDF */ TODO return(-1); case XEN_SCHEDULER_CREDIT: - if (*nparams < 2) - return(-1); + if (*nparams < XEN_SCHED_CRED_NPARAM) { + virXenError(VIR_ERR_INVALID_ARG, + "%s", _("Invalid parameter count")); + return -1; + } memset(&op_dom, 0, sizeof(op_dom)); op_dom.cmd = XEN_V2_OP_SCHEDULER; op_dom.domain = (domid_t) domain->id; @@ -1319,7 +1328,7 @@ xenHypervisorGetSchedulerParameters(virDomainPtr domain, params[1].type = VIR_DOMAIN_SCHED_FIELD_UINT; params[1].value.ui = op_dom.u.getschedinfo.u.credit.cap; - *nparams = 2; + *nparams = XEN_SCHED_CRED_NPARAM; break; default: virXenErrorFunc(VIR_ERR_INVALID_ARG, __FUNCTION__, diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 359d5f4..0fa8042 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -55,8 +55,6 @@ /* * The number of Xen scheduler parameters */ -#define XEN_SCHED_SEDF_NPARAM 6 -#define XEN_SCHED_CRED_NPARAM 2 #define XEND_RCV_BUF_MAX_LEN 65536 @@ -3607,8 +3605,7 @@ xenDaemonGetSchedulerParameters(virDomainPtr domain, int sched_nparam = 0; int ret = -1; - if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL) - || (params == NULL) || (nparams == NULL)) { + if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL)) { virXendError(VIR_ERR_INVALID_ARG, __FUNCTION__); return (-1); } @@ -3636,10 +3633,22 @@ xenDaemonGetSchedulerParameters(virDomainPtr domain, switch (sched_nparam){ case XEN_SCHED_SEDF_NPARAM: + if (*nparams < XEN_SCHED_SEDF_NPARAM) { + virXendError(VIR_ERR_INVALID_ARG, + "%s", _("Invalid parameter count")); + goto error; + } + /* TODO: Implement for Xen/SEDF */ TODO goto error; case XEN_SCHED_CRED_NPARAM: + if (*nparams < XEN_SCHED_CRED_NPARAM) { + virXendError(VIR_ERR_INVALID_ARG, + "%s", _("Invalid parameter count")); + goto error; + } + /* get cpu_weight/cpu_cap from xend/domain */ if (sexpr_node(root, "domain/cpu_weight") == NULL) { virXendError(VIR_ERR_INTERNAL_ERROR, -- 1.7.0.4
On Wed, May 18, 2011 at 12:21:09PM +0200, Matthias Bolte wrote:
params and nparams are essential and cannot be NULL. Check this in libvirt.c and remove redundant checks from the drivers (e.g. xend).
Instead of enforcing that nparams must point to exact same value as returned by virDomainGetSchedulerType relax this to a lower bound check. This is what some drivers (e.g. xen hypervisor and esx) already did. Other drivers (e.g. xend) didn't check nparams at all and assumed that there is enough space in params.
Unify the behavior in all drivers to a lower bound check and update nparams to the number of valid values in params on success. [...] - * Get the scheduler parameters, the @params array will be filled with the - * values. + * Get all scheduler parameters, the @params array will be filled with the + * values and @nparams will be updated to the number of valid elements in + * @params. * * Returns -1 in case of error, 0 in case of success. */ @@ -5041,6 +5042,12 @@ virDomainGetSchedulerParameters(virDomainPtr domain, virDispatchError(NULL); return -1; } + + if (params == NULL || nparams == NULL || *nparams < 0) {
Hum, what would *nparams == 0 mean ? The arry pointer has to be allocated anyway. Maybe we should use *nparams <= 0 here
+ virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + conn = domain->conn;
if (conn->driver->domainGetSchedulerParameters) { diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 6b2d806..b85c743 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2466,7 +2466,7 @@ libxlDomainGetSchedulerParameters(virDomainPtr dom, virSchedParameterPtr params, goto cleanup; }
- if (*nparams != XEN_SCHED_CREDIT_NPARAM) { + if (*nparams < XEN_SCHED_CREDIT_NPARAM) { libxlError(VIR_ERR_INVALID_ARG, "%s", _("Invalid parameter count")); goto cleanup; } @@ -2494,6 +2494,7 @@ libxlDomainGetSchedulerParameters(virDomainPtr dom, virSchedParameterPtr params, goto cleanup; }
+ *nparams = XEN_SCHED_CREDIT_NPARAM; ret = 0;
cleanup: diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 2bb592d..a65299b 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2234,7 +2234,7 @@ static int lxcGetSchedulerParameters(virDomainPtr domain, if (driver->cgroup == NULL) return -1;
- if ((*nparams) != 1) { + if (*nparams < 1) {
which is what we are doing here and in all the drivers. ACK, with that nit 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 Wed, May 18, 2011 at 12:21:09PM +0200, Matthias Bolte wrote:
params and nparams are essential and cannot be NULL. Check this in libvirt.c and remove redundant checks from the drivers (e.g. xend).
Instead of enforcing that nparams must point to exact same value as returned by virDomainGetSchedulerType relax this to a lower bound check. This is what some drivers (e.g. xen hypervisor and esx) already did. Other drivers (e.g. xend) didn't check nparams at all and assumed that there is enough space in params.
Unify the behavior in all drivers to a lower bound check and update nparams to the number of valid values in params on success. [...] - * Get the scheduler parameters, the @params array will be filled with the - * values. + * Get all scheduler parameters, the @params array will be filled with the + * values and @nparams will be updated to the number of valid elements in + * @params. * * Returns -1 in case of error, 0 in case of success. */ @@ -5041,6 +5042,12 @@ virDomainGetSchedulerParameters(virDomainPtr domain, virDispatchError(NULL); return -1; } + + if (params == NULL || nparams == NULL || *nparams < 0) {
Hum, what would *nparams == 0 mean ? The arry pointer has to be allocated anyway. Maybe we should use *nparams <= 0 here
True, I changed that before pushing the whole series.
+ virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + conn = domain->conn;
if (conn->driver->domainGetSchedulerParameters) { diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 6b2d806..b85c743 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2466,7 +2466,7 @@ libxlDomainGetSchedulerParameters(virDomainPtr dom, virSchedParameterPtr params, goto cleanup; }
- if (*nparams != XEN_SCHED_CREDIT_NPARAM) { + if (*nparams < XEN_SCHED_CREDIT_NPARAM) { libxlError(VIR_ERR_INVALID_ARG, "%s", _("Invalid parameter count")); goto cleanup; } @@ -2494,6 +2494,7 @@ libxlDomainGetSchedulerParameters(virDomainPtr dom, virSchedParameterPtr params, goto cleanup; }
+ *nparams = XEN_SCHED_CREDIT_NPARAM; ret = 0;
cleanup: diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 2bb592d..a65299b 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2234,7 +2234,7 @@ static int lxcGetSchedulerParameters(virDomainPtr domain, if (driver->cgroup == NULL) return -1;
- if ((*nparams) != 1) { + if (*nparams < 1) {
which is what we are doing here and in all the drivers.
ACK, with that nit
Daniel
Matthias
Add invalid argument checks for params and nparams to the public API and remove the from the drivers (e.g. xend). Add subset handling to libxl and test drivers. --- src/libvirt.c | 24 ++++++++++++++++++------ src/libxl/libxl_driver.c | 6 ++++-- src/test/test_driver.c | 27 +++++++++++++-------------- src/xen/xen_hypervisor.c | 7 +++---- src/xen/xend_internal.c | 3 +-- 5 files changed, 39 insertions(+), 28 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index e8b5b80..2c2866e 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -5069,11 +5069,11 @@ error: * virDomainSetSchedulerParameters: * @domain: pointer to domain object * @params: pointer to scheduler parameter objects - * @nparams: number of scheduler parameter - * (this value should be same or less than the returned value + * @nparams: number of scheduler parameter objects + * (this value can be the same or less than the returned value * nparams of virDomainGetSchedulerType) * - * Change the scheduler parameters + * Change all or a subset or the scheduler parameters. * * Returns -1 in case of error, 0 in case of success. */ @@ -5092,6 +5092,12 @@ virDomainSetSchedulerParameters(virDomainPtr domain, virDispatchError(NULL); return -1; } + + if (params == NULL || nparams < 0) { + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + if (domain->conn->flags & VIR_CONNECT_RO) { virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; @@ -5118,12 +5124,12 @@ error: * virDomainSetSchedulerParametersFlags: * @domain: pointer to domain object * @params: pointer to scheduler parameter objects - * @nparams: number of scheduler parameter - * (this value should be same or less than the returned value + * @nparams: number of scheduler parameter objects + * (this value can be the same or less than the returned value * nparams of virDomainGetSchedulerType) * @flags: virDomainSchedParameterFlags * - * Change the scheduler parameters + * Change a subset or all scheduler parameters. * * Returns -1 in case of error, 0 in case of success. */ @@ -5153,6 +5159,12 @@ virDomainSetSchedulerParametersFlags(virDomainPtr domain, virDispatchError(NULL); return -1; } + + if (params == NULL || nparams < 0) { + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + if (domain->conn->flags & VIR_CONNECT_RO) { virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); goto error; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index b85c743..1d41e2d 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2544,8 +2544,10 @@ libxlDomainSetSchedulerParameters(virDomainPtr dom, virSchedParameterPtr params, goto cleanup; } - if (nparams != XEN_SCHED_CREDIT_NPARAM) { - libxlError(VIR_ERR_INVALID_ARG, "%s", _("Invalid parameter count")); + if (libxl_sched_credit_domain_get(&priv->ctx, dom->id, &sc_info) != 0) { + libxlError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get scheduler parameters for domain '%d'" + " with libxenlight"), dom->id); goto cleanup; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 4897081..69aeb29 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2671,7 +2671,7 @@ static int testDomainSetSchedulerParams(virDomainPtr domain, { testConnPtr privconn = domain->conn->privateData; virDomainObjPtr privdom; - int ret = -1; + int ret = -1, i; testDriverLock(privconn); privdom = virDomainFindByName(&privconn->domains, @@ -2683,20 +2683,19 @@ static int testDomainSetSchedulerParams(virDomainPtr domain, goto cleanup; } - if (nparams != 1) { - testError(VIR_ERR_INVALID_ARG, "nparams"); - goto cleanup; - } - if (STRNEQ(params[0].field, "weight")) { - testError(VIR_ERR_INVALID_ARG, "field"); - goto cleanup; - } - if (params[0].type != VIR_DOMAIN_SCHED_FIELD_UINT) { - testError(VIR_ERR_INVALID_ARG, "type"); - goto cleanup; + for (i = 0; i < nparams; i++) { + if (STRNEQ(params[i].field, "weight")) { + testError(VIR_ERR_INVALID_ARG, "field"); + goto cleanup; + } + if (params[i].type != VIR_DOMAIN_SCHED_FIELD_UINT) { + testError(VIR_ERR_INVALID_ARG, "type"); + goto cleanup; + } + /* XXX */ + /*privdom->weight = params[i].value.ui;*/ } - /* XXX */ - /*privdom->weight = params[0].value.ui;*/ + ret = 0; cleanup: diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index 8d579bc..253164e 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -1364,10 +1364,9 @@ xenHypervisorSetSchedulerParameters(virDomainPtr domain, return -1; } - if ((nparams == 0) || (params == NULL)) { - virXenErrorFunc(VIR_ERR_INVALID_ARG, __FUNCTION__, - "Noparameters given", 0); - return(-1); + if (nparams == 0) { + /* nothing to do, exit early */ + return 0; } priv = (xenUnifiedPrivatePtr) domain->conn->privateData; diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 0fa8042..1a3c3b4 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -3712,8 +3712,7 @@ xenDaemonSetSchedulerParameters(virDomainPtr domain, int sched_nparam = 0; int ret = -1; - if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL) - || (params == NULL)) { + if ((domain == NULL) || (domain->conn == NULL) || (domain->name == NULL)) { virXendError(VIR_ERR_INVALID_ARG, __FUNCTION__); return (-1); } -- 1.7.0.4
On 05/18/2011 04:21 AM, Matthias Bolte wrote:
Add invalid argument checks for params and nparams to the public API and remove the from the drivers (e.g. xend).
Add subset handling to libxl and test drivers.
@@ -5092,6 +5092,12 @@ virDomainSetSchedulerParameters(virDomainPtr domain, virDispatchError(NULL); return -1; } + + if (params == NULL || nparams < 0) {
So this documents that nparams should be in the closed range [1, max]...
@@ -2683,20 +2683,19 @@ static int testDomainSetSchedulerParams(virDomainPtr domain, goto cleanup; }
- if (nparams != 1) { - testError(VIR_ERR_INVALID_ARG, "nparams"); - goto cleanup; - } - if (STRNEQ(params[0].field, "weight")) { - testError(VIR_ERR_INVALID_ARG, "field"); - goto cleanup; - } - if (params[0].type != VIR_DOMAIN_SCHED_FIELD_UINT) { - testError(VIR_ERR_INVALID_ARG, "type"); - goto cleanup; + for (i = 0; i < nparams; i++) { + if (STRNEQ(params[i].field, "weight")) {
Hmm, this change lets us pass an array of two separate weight changes, the last one wins, whereas prepatch the maximum was 1, so you could not pass two duplicate weights.
+++ b/src/xen/xen_hypervisor.c @@ -1364,10 +1364,9 @@ xenHypervisorSetSchedulerParameters(virDomainPtr domain, return -1; }
- if ((nparams == 0) || (params == NULL)) { - virXenErrorFunc(VIR_ERR_INVALID_ARG, __FUNCTION__, - "Noparameters given", 0); - return(-1); + if (nparams == 0) { + /* nothing to do, exit early */ + return 0;
Given the public check that nparams is not less than 1, this is now dead code. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2011/5/18 Eric Blake <eblake@redhat.com>:
On 05/18/2011 04:21 AM, Matthias Bolte wrote:
Add invalid argument checks for params and nparams to the public API and remove the from the drivers (e.g. xend).
Add subset handling to libxl and test drivers.
@@ -5092,6 +5092,12 @@ virDomainSetSchedulerParameters(virDomainPtr domain, virDispatchError(NULL); return -1; } + + if (params == NULL || nparams < 0) {
So this documents that nparams should be in the closed range [1, max]...
@@ -2683,20 +2683,19 @@ static int testDomainSetSchedulerParams(virDomainPtr domain, goto cleanup; }
- if (nparams != 1) { - testError(VIR_ERR_INVALID_ARG, "nparams"); - goto cleanup; - } - if (STRNEQ(params[0].field, "weight")) { - testError(VIR_ERR_INVALID_ARG, "field"); - goto cleanup; - } - if (params[0].type != VIR_DOMAIN_SCHED_FIELD_UINT) { - testError(VIR_ERR_INVALID_ARG, "type"); - goto cleanup; + for (i = 0; i < nparams; i++) { + if (STRNEQ(params[i].field, "weight")) {
Hmm, this change lets us pass an array of two separate weight changes, the last one wins, whereas prepatch the maximum was 1, so you could not pass two duplicate weights.
Reminds me that I wanted to add a comment about duplicate parameters to the setter functions.
From a practical point of view this change didn't alter the behavior of the test driver as the weight is hardcoded to 50 anyway :)
+++ b/src/xen/xen_hypervisor.c @@ -1364,10 +1364,9 @@ xenHypervisorSetSchedulerParameters(virDomainPtr domain, return -1; }
- if ((nparams == 0) || (params == NULL)) { - virXenErrorFunc(VIR_ERR_INVALID_ARG, __FUNCTION__, - "Noparameters given", 0); - return(-1); + if (nparams == 0) { + /* nothing to do, exit early */ + return 0;
Given the public check that nparams is not less than 1, this is now dead code.
Yep, I missed to removed that check when changing the check from nparams < 0 to nparams <= 0. Matthias
--- src/libvirt.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 2c2866e..3b41f1d 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -2992,11 +2992,11 @@ error: * virDomainSetMemoryParameters: * @domain: pointer to domain object * @params: pointer to memory parameter objects - * @nparams: number of memory parameter (this value should be same or + * @nparams: number of memory parameter (this value can be the same or * less than the number of parameters supported) * @flags: currently unused, for future extension * - * Change the memory tunables + * Change all or a subset of the memory tunables. * This function requires privileged access to the hypervisor. * * Returns -1 in case of error, 0 in case of success. @@ -3118,11 +3118,11 @@ error: * virDomainSetBlkioParameters: * @domain: pointer to domain object * @params: pointer to blkio parameter objects - * @nparams: number of blkio parameters (this value should be same or + * @nparams: number of blkio parameters (this value can be the same or * less than the number of parameters supported) * @flags: currently unused, for future extension * - * Change the blkio tunables + * Change all or a subset of the blkio tunables. * This function requires privileged access to the hypervisor. * * Returns -1 in case of error, 0 in case of success. -- 1.7.0.4
Improve invalid argument checks in the size query case. The drivers already relied on this unchecked behavior. Relax the implementation of virDomainGet(Memory|Blkio)MemoryParameters in the drivers and allow to pass more memory than necessary for all parameters. --- src/libvirt.c | 13 ++++++++----- src/lxc/lxc_driver.c | 5 +++-- src/qemu/qemu_driver.c | 5 +++-- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 3b41f1d..d6e222e 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3051,7 +3051,7 @@ error: * @nparams: pointer to number of memory parameters * @flags: currently unused, for future extension * - * Get the memory parameters, the @params array will be filled with the values + * Get all memory parameters, the @params array will be filled with the values * equal to the number of parameters suggested by @nparams * * As the value of @nparams is dynamic, call the API setting @nparams to 0 and @@ -3094,7 +3094,8 @@ virDomainGetMemoryParameters(virDomainPtr domain, virDispatchError(NULL); return -1; } - if ((nparams == NULL) || (*nparams < 0)) { + if ((nparams == NULL) || (*nparams < 0) || + (params == NULL && *nparams != 0)) { virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; } @@ -3177,8 +3178,9 @@ error: * @nparams: pointer to number of blkio parameters * @flags: currently unused, for future extension * - * Get the blkio parameters, the @params array will be filled with the values - * equal to the number of parameters suggested by @nparams + * Get all blkio parameters, the @params array will be filled with the values + * equal to the number of parameters suggested by @nparams. + * See virDomainGetMemoryParameters for an equivalent usage example. * * This function requires privileged access to the hypervisor. This function * expects the caller to allocate the @params. @@ -3202,7 +3204,8 @@ virDomainGetBlkioParameters(virDomainPtr domain, virDispatchError(NULL); return -1; } - if ((nparams == NULL) || (*nparams < 0)) { + if ((nparams == NULL) || (*nparams < 0) || + (params == NULL && *nparams != 0)) { virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; } diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index a65299b..9e09c95 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -871,7 +871,7 @@ static int lxcDomainGetMemoryParameters(virDomainPtr dom, ret = 0; goto cleanup; } - if ((*nparams) != LXC_NB_MEM_PARAM) { + if ((*nparams) < LXC_NB_MEM_PARAM) { lxcError(VIR_ERR_INVALID_ARG, "%s", _("Invalid parameter count")); goto cleanup; @@ -883,7 +883,7 @@ static int lxcDomainGetMemoryParameters(virDomainPtr dom, goto cleanup; } - for (i = 0; i < *nparams; i++) { + for (i = 0; i < LXC_NB_MEM_PARAM; i++) { virMemoryParameterPtr param = ¶ms[i]; val = 0; param->value.ul = 0; @@ -941,6 +941,7 @@ static int lxcDomainGetMemoryParameters(virDomainPtr dom, } } + *nparams = LXC_NB_MEM_PARAM; ret = 0; cleanup: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9a7286d..2f9c8e7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4957,7 +4957,7 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, goto cleanup; } - if ((*nparams) != QEMU_NB_MEM_PARAM) { + if ((*nparams) < QEMU_NB_MEM_PARAM) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid parameter count")); goto cleanup; @@ -4969,7 +4969,7 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, goto cleanup; } - for (i = 0; i < *nparams; i++) { + for (i = 0; i < QEMU_NB_MEM_PARAM; i++) { virMemoryParameterPtr param = ¶ms[i]; val = 0; param->value.ul = 0; @@ -5027,6 +5027,7 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, } } + *nparams = QEMU_NB_MEM_PARAM; ret = 0; cleanup: -- 1.7.0.4
--- src/libvirt.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 76 insertions(+), 6 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index d6e222e..a423e77 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; } @@ -4490,7 +4490,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, @@ -5621,7 +5620,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; } @@ -6527,6 +6526,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; @@ -6587,6 +6592,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; @@ -6632,6 +6643,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; @@ -6688,6 +6705,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; @@ -6748,6 +6771,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; @@ -7408,7 +7437,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; } @@ -8274,7 +8303,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; } @@ -8717,7 +8746,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; } @@ -9789,6 +9818,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; @@ -9845,6 +9879,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__); @@ -10595,6 +10634,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); @@ -11853,6 +11897,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; @@ -11948,6 +11997,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; @@ -12024,6 +12078,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")); @@ -12121,6 +12180,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")); @@ -13835,6 +13899,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 Wed, May 18, 2011 at 12:21:07PM +0200, Matthias Bolte wrote:
This series was inspired by the discusion about invalid argument checking in the public API and the unclear semantic of this functions.
https://www.redhat.com/archives/libvir-list/2011-May/msg01105.html
Clarify that the getter functions don't allow to obtain a subset of the parameters. You can only get all at once.
Clarify that the setter functions can set a subset of the parameters.
ACK to the serie with small nit in 2/6 , also 6/6 looks a lot like one of the previous patches in the previous patch set, but seems okay to me ! 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 Wed, May 18, 2011 at 12:21:07PM +0200, Matthias Bolte wrote:
This series was inspired by the discusion about invalid argument checking in the public API and the unclear semantic of this functions.
https://www.redhat.com/archives/libvir-list/2011-May/msg01105.html
Clarify that the getter functions don't allow to obtain a subset of the parameters. You can only get all at once.
Clarify that the setter functions can set a subset of the parameters.
ACK to the serie with small nit in 2/6 , also 6/6 looks a lot like one of the previous patches in the previous patch set, but seems okay to me !
Daniel
Yes, 6/6 is the rest of the original patch that led to this series, I should have denoted that. I changed 2/6 as suggested and pushed the series. Matthias
participants (3)
-
Daniel Veillard -
Eric Blake -
Matthias Bolte