[libvirt] [PATCH v3 0/7] add new API virConnectGetCPUModelNames
This series adds a new API "virConnectGetCPUModelNames" that allows to retrieve the list of CPU models known by the hypervisor for a specific architecture. This new function is mainly needed by virt-manager to not read directly the cpu_map.xml file (it could also be different when accessing a remote daemon). I have amended all the comments reported for v2. *v3 main changes - virConnectGetCPUModelNames returns the number of models instead of 0 on success. - Use VIR_INSERT_ELEMENT instead of VIR_EXPAND_N. - Fix a potential memory leak in the python bindings. - Move virsh changes to a separate commit. - Remove API documentation from libvirt.h. *v2 main changes - set a hard limit for the number of CPU models that is possible to fetch from a remote server. - Use VIR_EXPAND_N instead of VIR_REALLOC_N. - s|1.1.2|1.1.3| Giuseppe Scrivano (7): libvirt: add new public API virConnectGetCPUModelNames cpu: add function to get the models for an arch virConnectGetCPUModelNames: implement the remote protocol virConnectGetCPUModelNames: add the support for qemu virConnectGetCPUModelNames: add the support for the test protocol virsh: add function to get the CPU models for an arch python: add bindings for virConnectGetCPUModelNames daemon/remote.c | 43 +++++++++++++++++++++++++++++++ include/libvirt/libvirt.h.in | 4 +++ python/generator.py | 1 + python/libvirt-override-api.xml | 7 +++++ python/libvirt-override.c | 52 +++++++++++++++++++++++++++++++++++++ python/libvirt-override.py | 11 ++++++++ src/cpu/cpu.c | 56 ++++++++++++++++++++++++++++++++++++++++ src/cpu/cpu.h | 3 +++ src/driver.h | 7 +++++ src/libvirt.c | 46 +++++++++++++++++++++++++++++++++ src/libvirt_private.syms | 1 + src/libvirt_public.syms | 5 ++++ src/qemu/qemu_driver.c | 14 ++++++++++ src/remote/remote_driver.c | 57 +++++++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 20 ++++++++++++++- src/remote_protocol-structs | 11 ++++++++ src/test/test_driver.c | 11 ++++++++ tools/virsh-host.c | 54 ++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 5 ++++ 19 files changed, 407 insertions(+), 1 deletion(-) -- 1.8.3.1
The new function virConnectGetCPUModelNames allows to retrieve the list of CPU models known by the hypervisor for a specific architecture. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- include/libvirt/libvirt.h.in | 4 ++++ python/generator.py | 1 + src/driver.h | 7 +++++++ src/libvirt.c | 46 ++++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 +++++ 5 files changed, 63 insertions(+) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index a47e33c..e27150d 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -4006,6 +4006,10 @@ int virConnectCompareCPU(virConnectPtr conn, const char *xmlDesc, unsigned int flags); +int virConnectGetCPUModelNames(virConnectPtr conn, + const char *arch, + char ***models, + unsigned int flags); /** * virConnectBaselineCPUFlags diff --git a/python/generator.py b/python/generator.py index a91dde8..73107d7 100755 --- a/python/generator.py +++ b/python/generator.py @@ -366,6 +366,7 @@ foreign_encoding_args = ( # Class methods which are written by hand in libvirt.c but the Python-level # code is still automatically generated (so they are not in skip_function()). skip_impl = ( + "virConnectGetCPUModelNames", 'virConnectGetVersion', 'virConnectGetLibVersion', 'virConnectListDomainsID', diff --git a/src/driver.h b/src/driver.h index be64333..8cd164a 100644 --- a/src/driver.h +++ b/src/driver.h @@ -682,6 +682,12 @@ typedef char * unsigned int flags); typedef int +(*virDrvConnectGetCPUModelNames)(virConnectPtr conn, + const char *args, + char ***models, + unsigned int flags); + +typedef int (*virDrvDomainGetJobInfo)(virDomainPtr domain, virDomainJobInfoPtr info); @@ -1332,6 +1338,7 @@ struct _virDriver { virDrvDomainMigratePerform3Params domainMigratePerform3Params; virDrvDomainMigrateFinish3Params domainMigrateFinish3Params; virDrvDomainMigrateConfirm3Params domainMigrateConfirm3Params; + virDrvConnectGetCPUModelNames connectGetCPUModelNames; }; diff --git a/src/libvirt.c b/src/libvirt.c index 665b30b..6d09cbd 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -18519,6 +18519,52 @@ error: /** + * virConnectGetCPUModelNames: + * + * @conn: virConnect connection + * @arch: Architecture + * @models: NULL terminated array of the CPU models supported for the specified + * architecture. Each element and the array itself must be freed by the caller + * with free. + * @flags: extra flags; not used yet, so callers should always pass 0. + * + * Get the list of supported CPU models for a specific architecture. + * + * Returns -1 on error, number of elements in @models on success. + */ +int +virConnectGetCPUModelNames(virConnectPtr conn, const char *arch, char ***models, + unsigned int flags) +{ + VIR_DEBUG("conn=%p, arch=%s, flags=%x", conn, arch, flags); + virResetLastError(); + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + virCheckNonNullArgReturn(arch, -1); + + if (conn->driver->connectGetCPUModelNames) { + int ret; + + ret = conn->driver->connectGetCPUModelNames(conn, arch, models, flags); + if (ret < 0) + goto error; + + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(conn); + return -1; +} + + +/** * virConnectBaselineCPU: * * @conn: virConnect connection diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index bbdf78a..fe9b497 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -634,4 +634,9 @@ LIBVIRT_1.1.1 { virDomainSetMemoryStatsPeriod; } LIBVIRT_1.1.0; +LIBVIRT_1.1.3 { + global: + virConnectGetCPUModelNames; +} LIBVIRT_1.1.1; + # .... define new API here using predicted next version number .... -- 1.8.3.1
On 09/11/2013 08:12 AM, Giuseppe Scrivano wrote:
The new function virConnectGetCPUModelNames allows to retrieve the list of CPU models known by the hypervisor for a specific architecture.
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> ---
/** + * virConnectGetCPUModelNames: + * + * @conn: virConnect connection + * @arch: Architecture + * @models: NULL terminated array of the CPU models supported for the specified + * architecture. Each element and the array itself must be freed by the caller + * with free.
Minor tweak: @models: Pointer to a variable to store the NULL-terminated array...
+ * @flags: extra flags; not used yet, so callers should always pass 0. + * + * Get the list of supported CPU models for a specific architecture. + * + * Returns -1 on error, number of elements in @models on success. + */ +int +virConnectGetCPUModelNames(virConnectPtr conn, const char *arch, char ***models, + unsigned int flags) +{ + VIR_DEBUG("conn=%p, arch=%s, flags=%x", conn, arch, flags); + virResetLastError(); + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + virCheckNonNullArgReturn(arch, -1);
Can models usefully be NULL, just for the return value without actually getting the number of names? I'll decide that based on the patch to the .x file... /me goes and looks Hmm, patch 3/7 blindly stores into *models, meaning you require a non-NULL models, and are missing a virCheckNonNullArgReturn(models, -1) here. On the other hand, it is inconsistent with all our other *ListAll functions, which allow a NULL pointer for the sake of using just the return value to grab a count of valid information (and implemented in the .x file by a side channel need_results in the _args, and ret in the _ret struct). I'm leaning towards consistency, so I'll support that here, too. Everything else looks good. ACK with this squashed in: diff --git i/src/libvirt.c w/src/libvirt.c index 159d922..b31b561 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -18528,9 +18528,10 @@ error: * * @conn: virConnect connection * @arch: Architecture - * @models: NULL terminated array of the CPU models supported for the specified - * architecture. Each element and the array itself must be freed by the caller - * with free. + * @models: Pointer to a variable to store the NULL-terminated array of the + * CPU models supported for the specified architecture. Each element + * and the array itself must be freed by the caller with free. Pass + * NULL if only the list length is needed. * @flags: extra flags; not used yet, so callers should always pass 0. * * Get the list of supported CPU models for a specific architecture. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/11/2013 08:12 AM, Giuseppe Scrivano wrote:
The new function virConnectGetCPUModelNames allows to retrieve the list of CPU models known by the hypervisor for a specific architecture.
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> ---
+int +virConnectGetCPUModelNames(virConnectPtr conn, const char *arch, char ***models, + unsigned int flags) +{ + VIR_DEBUG("conn=%p, arch=%s, flags=%x", conn, arch, flags); + virResetLastError(); + + if (!VIR_IS_CONNECT(conn)) { + virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); + virDispatchError(NULL); + return -1;
Oh, I also missed another point of consistency with our ListAll functions - we guarantee that if models is non-NULL, it will be sanitized on all error paths. Squashing this in, too: diff --git i/src/libvirt.c w/src/libvirt.c index b31b561..b9b9459 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -18545,6 +18545,9 @@ virConnectGetCPUModelNames(virConnectPtr conn, const char *arch, char ***models, VIR_DEBUG("conn=%p, arch=%s, flags=%x", conn, arch, flags); virResetLastError(); + if (models) + *models = NULL; + if (!VIR_IS_CONNECT(conn)) { virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__); virDispatchError(NULL); -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/13/2013 01:25 PM, Eric Blake wrote:
On 09/11/2013 08:12 AM, Giuseppe Scrivano wrote:
The new function virConnectGetCPUModelNames allows to retrieve the list of CPU models known by the hypervisor for a specific architecture.
@@ -18545,6 +18545,9 @@ virConnectGetCPUModelNames(virConnectPtr conn, const char *arch, char ***models, VIR_DEBUG("conn=%p, arch=%s, flags=%x", conn, arch, flags);
And yet another tweak squashed in - we must log ALL parameters. diff --git i/src/libvirt.c w/src/libvirt.c index b9b9459..5831f84 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -18542,7 +18542,8 @@ int virConnectGetCPUModelNames(virConnectPtr conn, const char *arch, char ***models, unsigned int flags) { - VIR_DEBUG("conn=%p, arch=%s, flags=%x", conn, arch, flags); + VIR_DEBUG("conn=%p, arch=%s, models=%p, flags=%x", + conn, arch, models, flags); virResetLastError(); if (models) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/cpu/cpu.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++ src/cpu/cpu.h | 3 +++ src/libvirt_private.syms | 1 + 3 files changed, 60 insertions(+) diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index 50c2de3..7011b3d 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -27,11 +27,13 @@ #include "viralloc.h" #include "virxml.h" #include "cpu.h" +#include "cpu_map.h" #include "cpu_x86.h" #include "cpu_powerpc.h" #include "cpu_s390.h" #include "cpu_arm.h" #include "cpu_generic.h" +#include "util/virstring.h" #define NR_DRIVERS ARRAY_CARDINALITY(drivers) @@ -456,3 +458,57 @@ cpuModelIsAllowed(const char *model, } return false; } + +struct cpuGetModelsData +{ + char **data; + size_t len; /* It includes the last element of DATA, which is NULL. */ +}; + +static int +cpuGetArchModelsCb(enum cpuMapElement element, + xmlXPathContextPtr ctxt, + void *cbdata) +{ + char *name; + struct cpuGetModelsData *data = cbdata; + if (element != CPU_MAP_ELEMENT_MODEL) + return 0; + + name = virXPathString("string(@name)", ctxt); + if (name == NULL) + return -1; + + return VIR_INSERT_ELEMENT(data->data, data->len - 1, data->len, name); +} + + +static int +cpuGetArchModels(const char *arch, struct cpuGetModelsData *data) +{ + return cpuMapLoad(arch, cpuGetArchModelsCb, data); +} + + +int +cpuGetModels(const char *arch, char ***models) +{ + struct cpuGetModelsData data; + + *models = data.data = NULL; + data.len = 1; + + if (VIR_ALLOC_N(data.data, data.len) < 0) + goto error; + + if (cpuGetArchModels(arch, &data) < 0) + goto error; + + *models = data.data; + + return data.len - 1; + +error: + virStringFreeList(data.data); + return -1; +} diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h index 69f6fae..ebbc7c1 100644 --- a/src/cpu/cpu.h +++ b/src/cpu/cpu.h @@ -175,4 +175,7 @@ cpuModelIsAllowed(const char *model, const char **models, unsigned int nmodels); +extern int +cpuGetModels(const char *arch, char ***models); + #endif /* __VIR_CPU_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 35f0f1b..b456c2d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -719,6 +719,7 @@ cpuCompareXML; cpuDataFree; cpuDecode; cpuEncode; +cpuGetModels; cpuGuestData; cpuHasFeature; cpuMapOverride; -- 1.8.3.1
On 09/11/2013 08:12 AM, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/cpu/cpu.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++ src/cpu/cpu.h | 3 +++ src/libvirt_private.syms | 1 + 3 files changed, 60 insertions(+)
+ +int +cpuGetModels(const char *arch, char ***models) +{ + struct cpuGetModelsData data; + + *models = data.data = NULL;
Blind dereference of models; but since I argued in patch 1 that passing NULL makes sense for the return value count, this needs a tweak... ACK with this squashed in. diff --git i/src/cpu/cpu.c w/src/cpu/cpu.c index 7011b3d..dc0c474 100644 --- i/src/cpu/cpu.c +++ w/src/cpu/cpu.c @@ -1,7 +1,7 @@ /* * cpu.c: internal functions for CPU manipulation * - * Copyright (C) 2009-2012 Red Hat, Inc. + * Copyright (C) 2009-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -479,6 +479,11 @@ cpuGetArchModelsCb(enum cpuMapElement element, if (name == NULL) return -1; + if (!data->data) { + VIR_FREE(name); + data->len++; + return 0; + } return VIR_INSERT_ELEMENT(data->data, data->len - 1, data->len, name); } @@ -495,16 +500,17 @@ cpuGetModels(const char *arch, char ***models) { struct cpuGetModelsData data; - *models = data.data = NULL; + data.data = NULL; data.len = 1; - if (VIR_ALLOC_N(data.data, data.len) < 0) + if (models && VIR_ALLOC_N(data.data, data.len) < 0) goto error; if (cpuGetArchModels(arch, &data) < 0) goto error; - *models = data.data; + if (models) + *models = data.data; return data.len - 1; -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/13/2013 01:29 PM, Eric Blake wrote:
On 09/11/2013 08:12 AM, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/cpu/cpu.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++ src/cpu/cpu.h | 3 +++ src/libvirt_private.syms | 1 + 3 files changed, 60 insertions(+)
+ +int +cpuGetModels(const char *arch, char ***models) +{ + struct cpuGetModelsData data; + + *models = data.data = NULL;
Blind dereference of models; but since I argued in patch 1 that passing NULL makes sense for the return value count, this needs a tweak...
ACK with this squashed in.
NACK, needs a v4; this is where we need to fix things to do the right subdriver mapping. Quoting IRC: and I know which one to change [15:30] jdenemar eblake: cpu-models should work for both x86 and x86_64 imho [15:31] eblake yep - where the map file lists x86, we need the cpu-models to support it for both i686 and x86_64 [15:32] jdenemar yeah, the x86 in cpu_map.xml is actually a cpu driver name and the driver has a list of archs it supports [15:34] eblake jdenemar: giuseppe_s used cpuMapLoad(arch, ...) - which is only doing a literal string match [15:34] eblake so where do we reverse map the driver names in the map file into actual arch names? [15:35] eblake or, where SHOULD we be doing that mapping? [15:38] jdenemar every cpu api in cpu.c calls cpuGetSubDriver to get the driver from a real arch [15:41] jdenemar so there should be a high level cpu api that takes a real arch and gives model names, that should look up the appropriate sub driver, call its api and it should load its section of cpu_map.xml and return the list -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Eric Blake <eblake@redhat.com> writes:
NACK, needs a v4; this is where we need to fix things to do the right subdriver mapping. Quoting IRC:
and I know which one to change [15:30] jdenemar eblake: cpu-models should work for both x86 and x86_64 imho [15:31] eblake yep - where the map file lists x86, we need the cpu-models to support it for both i686 and x86_64 [15:32] jdenemar yeah, the x86 in cpu_map.xml is actually a cpu driver name and the driver has a list of archs it supports [15:34] eblake jdenemar: giuseppe_s used cpuMapLoad(arch, ...) - which is only doing a literal string match [15:34] eblake so where do we reverse map the driver names in the map file into actual arch names? [15:35] eblake or, where SHOULD we be doing that mapping? [15:38] jdenemar every cpu api in cpu.c calls cpuGetSubDriver to get the driver from a real arch [15:41] jdenemar so there should be a high level cpu api that takes a real arch and gives model names, that should look up the appropriate sub driver, call its api and it should load its section of cpu_map.xml and return the list
Jiri, would something like this suffice or am I missing some other details? Thanks, Giuseppe +int +cpuGetModels(const char *archName, char ***models) +{ + struct cpuGetModelsData data; + virArch arch; + struct cpuArchDriver *driver; + data.data = NULL; + data.len = 1; + + arch = virArchFromString(archName); + if (arch == VIR_ARCH_NONE) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot find architecture %s"), + archName); + goto error; + } + + driver = cpuGetSubDriver(arch); + if (driver == NULL) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot find a driver for the architecture %s"), + archName); + goto error; + } + + if (models && VIR_ALLOC_N(data.data, data.len) < 0) + goto error; + + if (cpuGetArchModels(driver->name, &data) < 0) + goto error; + + if (models) + *models = data.data; + + return data.len - 1; + +error: + virStringFreeList(data.data); + return -1; +}
On Sat, Sep 14, 2013 at 15:07:49 +0200, Giuseppe Scrivano wrote:
Eric Blake <eblake@redhat.com> writes:
NACK, needs a v4; this is where we need to fix things to do the right subdriver mapping. Quoting IRC:
and I know which one to change [15:30] jdenemar eblake: cpu-models should work for both x86 and x86_64 imho [15:31] eblake yep - where the map file lists x86, we need the cpu-models to support it for both i686 and x86_64 [15:32] jdenemar yeah, the x86 in cpu_map.xml is actually a cpu driver name and the driver has a list of archs it supports [15:34] eblake jdenemar: giuseppe_s used cpuMapLoad(arch, ...) - which is only doing a literal string match [15:34] eblake so where do we reverse map the driver names in the map file into actual arch names? [15:35] eblake or, where SHOULD we be doing that mapping? [15:38] jdenemar every cpu api in cpu.c calls cpuGetSubDriver to get the driver from a real arch [15:41] jdenemar so there should be a high level cpu api that takes a real arch and gives model names, that should look up the appropriate sub driver, call its api and it should load its section of cpu_map.xml and return the list
Jiri, would something like this suffice or am I missing some other details?
Thanks, Giuseppe
+int +cpuGetModels(const char *archName, char ***models) +{ + struct cpuGetModelsData data; + virArch arch; + struct cpuArchDriver *driver; + data.data = NULL; + data.len = 1; + + arch = virArchFromString(archName); + if (arch == VIR_ARCH_NONE) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot find architecture %s"), + archName); + goto error; + }
I'm not sure if the API should take a string and call virArchFromString or if that part should be done by the caller and this API would just take virArch. I guess it doesn't really matter until we need to reuse this API in a context where we don't have a string arch.
+ + driver = cpuGetSubDriver(arch); + if (driver == NULL) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot find a driver for the architecture %s"), + archName); + goto error; + }
I was rather thinking about adding a new driver API so that you'd just do return driver->getModels(models); here, similarly to all other cpu APIs. And the per-arch API would just use it's existing functions to load the CPU map and take the list of models from the parsed map. But the way you do it will work too. It's just that I'd feel better if all arch specific things were located in sub drivers. Even though this API is somewhere between arch specific and arch agnostic :-)
+ + if (models && VIR_ALLOC_N(data.data, data.len) < 0) + goto error; + + if (cpuGetArchModels(driver->name, &data) < 0) + goto error; + + if (models) + *models = data.data; + + return data.len - 1; + +error: + virStringFreeList(data.data); + return -1; +}
Jirka
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- daemon/remote.c | 43 +++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 57 ++++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 20 +++++++++++++++- src/remote_protocol-structs | 11 +++++++++ 4 files changed, 130 insertions(+), 1 deletion(-) diff --git a/daemon/remote.c b/daemon/remote.c index 2aff7c1..d357016 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5037,6 +5037,49 @@ cleanup: static int +remoteDispatchConnectGetCPUModelNames(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_connect_get_cpu_model_names_args *args, + remote_connect_get_cpu_model_names_ret *ret) +{ + int len, rv = -1; + char **models; + struct daemonClientPrivate *priv = + virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + len = virConnectGetCPUModelNames(priv->conn, args->arch, &models, + args->flags); + if (len < 0) + goto cleanup; + + if (ret->models.models_len > REMOTE_CONNECT_CPU_MODELS_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many CPU models '%d' for limit '%d'"), + ret->models.models_len, REMOTE_CONNECT_CPU_MODELS_MAX); + virStringFreeList(models); + goto cleanup; + } + + ret->models.models_val = models; + ret->models.models_len = len; + + rv = 0; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + return rv; +} + + +static int remoteDispatchDomainCreateXMLWithFiles(virNetServerPtr server ATTRIBUTE_UNUSED, virNetServerClientPtr client, virNetMessagePtr msg ATTRIBUTE_UNUSED, diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 62e77a5..aa3762a 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -5541,6 +5541,62 @@ done: static int +remoteConnectGetCPUModelNames(virConnectPtr conn, + const char *arch, + char ***models, + unsigned int flags) +{ + int rv = -1; + size_t i; + char **retmodels; + remote_connect_get_cpu_model_names_args args; + remote_connect_get_cpu_model_names_ret ret; + struct private_data *priv = conn->privateData; + remoteDriverLock(priv); + + memset(&args, 0, sizeof(args)); + memset(&ret, 0, sizeof(ret)); + + args.arch = (char *) arch; + args.flags = flags; + + if (call(conn, priv, 0, REMOTE_PROC_CONNECT_GET_CPU_MODEL_NAMES, + (xdrproc_t) xdr_remote_connect_get_cpu_model_names_args, + (char *) &args, + (xdrproc_t) xdr_remote_connect_get_cpu_model_names_ret, + (char *) &ret) < 0) + goto error; + + /* Check the length of the returned list carefully. */ + if (ret.models.models_len > REMOTE_CONNECT_CPU_MODELS_MAX) { + virReportError(VIR_ERR_RPC, "%s", + _("remoteConnectGetCPUModelNames: " + "returned number of CPU models exceeds limit")); + goto error; + } + + if (VIR_ALLOC_N(retmodels, ret.models.models_len + 1) < 0) + goto error; + + for (i = 0; i < ret.models.models_len; i++) + retmodels[i] = ret.models.models_val[i]; + + /* Caller frees MODELS. */ + *models = retmodels; + rv = ret.models.models_len; + +done: + remoteDriverUnlock(priv); + return rv; + +error: + rv = -1; + virStringFreeList(ret.models.models_val); + goto done; +} + + +static int remoteDomainOpenGraphics(virDomainPtr dom, unsigned int idx, int fd, @@ -6933,6 +6989,7 @@ static virDriver remote_driver = { .domainMigratePerform3Params = remoteDomainMigratePerform3Params, /* 1.1.0 */ .domainMigrateFinish3Params = remoteDomainMigrateFinish3Params, /* 1.1.0 */ .domainMigrateConfirm3Params = remoteDomainMigrateConfirm3Params, /* 1.1.0 */ + .connectGetCPUModelNames = remoteConnectGetCPUModelNames, /* 1.1.3 */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index a1c23da..a1d90ad 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -232,6 +232,9 @@ const REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX = 64; /* Upper limit on number of job stats */ const REMOTE_DOMAIN_JOB_STATS_MAX = 16; +/* Upper limit on number of CPU models */ +const REMOTE_CONNECT_CPU_MODELS_MAX = 8192; + /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ typedef opaque remote_uuid[VIR_UUID_BUFLEN]; @@ -2835,6 +2838,15 @@ struct remote_domain_event_device_removed_msg { remote_nonnull_string devAlias; }; +struct remote_connect_get_cpu_model_names_args { + remote_nonnull_string arch; + unsigned int flags; +}; + +struct remote_connect_get_cpu_model_names_ret { + remote_nonnull_string models<REMOTE_CONNECT_CPU_MODELS_MAX>; +}; + /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -4998,5 +5010,11 @@ enum remote_procedure { * @generate: both * @acl: none */ - REMOTE_PROC_DOMAIN_EVENT_DEVICE_REMOVED = 311 + REMOTE_PROC_DOMAIN_EVENT_DEVICE_REMOVED = 311, + + /** + * @generate: none + * @acl: connect:read + */ + REMOTE_PROC_CONNECT_GET_CPU_MODEL_NAMES = 312 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 4e27aae..9148202 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -2316,6 +2316,16 @@ struct remote_domain_event_device_removed_msg { remote_nonnull_domain dom; remote_nonnull_string devAlias; }; +struct remote_connect_get_cpu_model_names_args { + remote_nonnull_string arch; + u_int flags; +}; +struct remote_connect_get_cpu_model_names_ret { + struct { + u_int models_len; + remote_nonnull_string * models_val; + } models; +}; enum remote_procedure { REMOTE_PROC_CONNECT_OPEN = 1, REMOTE_PROC_CONNECT_CLOSE = 2, @@ -2628,4 +2638,5 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_CREATE_XML_WITH_FILES = 309, REMOTE_PROC_DOMAIN_CREATE_WITH_FILES = 310, REMOTE_PROC_DOMAIN_EVENT_DEVICE_REMOVED = 311, + REMOTE_PROC_CONNECT_GET_CPU_MODEL_NAMES = 312, }; -- 1.8.3.1
On 09/11/2013 08:12 AM, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- daemon/remote.c | 43 +++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 57 ++++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 20 +++++++++++++++- src/remote_protocol-structs | 11 +++++++++ 4 files changed, 130 insertions(+), 1 deletion(-)
static int +remoteDispatchConnectGetCPUModelNames(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr msg ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_connect_get_cpu_model_names_args *args, + remote_connect_get_cpu_model_names_ret *ret) +{ + int len, rv = -1; + char **models; + struct daemonClientPrivate *priv = + virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + len = virConnectGetCPUModelNames(priv->conn, args->arch, &models, + args->flags);
Needs a tweak to pass NULL on to the driver.
+ if (len < 0) + goto cleanup; + + if (ret->models.models_len > REMOTE_CONNECT_CPU_MODELS_MAX) {
Wrong variable; here, you want to check if we can encode 'len' into ret.
+ virReportError(VIR_ERR_RPC, + _("Too many CPU models '%d' for limit '%d'"), + ret->models.models_len, REMOTE_CONNECT_CPU_MODELS_MAX); + virStringFreeList(models); + goto cleanup; + } + + ret->models.models_val = models; + ret->models.models_len = len;
Doesn't work when len is 0 - in that case, models is non-NULL (space to a lone NULL pointer terminating the empty array), but xdr wants us to pass NULL, and we must still free models in that case.
+++ b/src/remote/remote_driver.c @@ -5541,6 +5541,62 @@ done:
static int +remoteConnectGetCPUModelNames(virConnectPtr conn, + const char *arch, + char ***models, + unsigned int flags) +{ + int rv = -1; + size_t i; + char **retmodels; + remote_connect_get_cpu_model_names_args args; + remote_connect_get_cpu_model_names_ret ret; + struct private_data *priv = conn->privateData; + remoteDriverLock(priv); + + memset(&args, 0, sizeof(args));
Pointless, since you end up assigning all members of args below.
+ memset(&ret, 0, sizeof(ret)); + + args.arch = (char *) arch; + args.flags = flags; + + if (call(conn, priv, 0, REMOTE_PROC_CONNECT_GET_CPU_MODEL_NAMES, + (xdrproc_t) xdr_remote_connect_get_cpu_model_names_args, + (char *) &args, + (xdrproc_t) xdr_remote_connect_get_cpu_model_names_ret, + (char *) &ret) < 0) + goto error; + + /* Check the length of the returned list carefully. */ + if (ret.models.models_len > REMOTE_CONNECT_CPU_MODELS_MAX) { + virReportError(VIR_ERR_RPC, "%s", + _("remoteConnectGetCPUModelNames: " + "returned number of CPU models exceeds limit"));
What limit? Other functions tell us the limit being violated.
+ goto error; + } + + if (VIR_ALLOC_N(retmodels, ret.models.models_len + 1) < 0) + goto error; + + for (i = 0; i < ret.models.models_len; i++) + retmodels[i] = ret.models.models_val[i]; + + /* Caller frees MODELS. */ + *models = retmodels;
Needs a tweak to avoid a blind NULL dereference.
+ rv = ret.models.models_len; + +done: + remoteDriverUnlock(priv); + return rv; + +error: + rv = -1;
Dead assignment; rv is already -1 if we get here.
+ virStringFreeList(ret.models.models_val);
Hmm. The moment we support a NULL models argument, we have to be careful that we free models_val always (not just on error); otherwise, a malicious program on the other end of the rpc pipe could ignore our request for not needing a result and still cause ret to contain an allocated list. I think a cleanup/done pair works for this better than a done/error pair, modelling after the ListAll methods.
+ goto done; +} + + +static int remoteDomainOpenGraphics(virDomainPtr dom, unsigned int idx, int fd, @@ -6933,6 +6989,7 @@ static virDriver remote_driver = { .domainMigratePerform3Params = remoteDomainMigratePerform3Params, /* 1.1.0 */ .domainMigrateFinish3Params = remoteDomainMigrateFinish3Params, /* 1.1.0 */ .domainMigrateConfirm3Params = remoteDomainMigrateConfirm3Params, /* 1.1.0 */ + .connectGetCPUModelNames = remoteConnectGetCPUModelNames, /* 1.1.3 */ };
static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index a1c23da..a1d90ad 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -232,6 +232,9 @@ const REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX = 64; /* Upper limit on number of job stats */ const REMOTE_DOMAIN_JOB_STATS_MAX = 16;
+/* Upper limit on number of CPU models */ +const REMOTE_CONNECT_CPU_MODELS_MAX = 8192; +
Seems a bit large given that we return just 2 for i686/x86_64 setups, but no real issue in keeping it that big.
/* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ typedef opaque remote_uuid[VIR_UUID_BUFLEN];
@@ -2835,6 +2838,15 @@ struct remote_domain_event_device_removed_msg { remote_nonnull_string devAlias; };
+struct remote_connect_get_cpu_model_names_args { + remote_nonnull_string arch; + unsigned int flags; +}; + +struct remote_connect_get_cpu_model_names_ret { + remote_nonnull_string models<REMOTE_CONNECT_CPU_MODELS_MAX>;
Needs a tweak to support the NULL models. ACK with this squashed in: diff --git i/daemon/remote.c w/daemon/remote.c index d357016..c8afa8f 100644 --- i/daemon/remote.c +++ w/daemon/remote.c @@ -5045,7 +5045,7 @@ remoteDispatchConnectGetCPUModelNames(virNetServerPtr server ATTRIBUTE_UNUSED, remote_connect_get_cpu_model_names_ret *ret) { int len, rv = -1; - char **models; + char **models = NULL; struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); @@ -5054,27 +5054,36 @@ remoteDispatchConnectGetCPUModelNames(virNetServerPtr server ATTRIBUTE_UNUSED, goto cleanup; } - len = virConnectGetCPUModelNames(priv->conn, args->arch, &models, + len = virConnectGetCPUModelNames(priv->conn, args->arch, + args->need_results ? &models : NULL, args->flags); if (len < 0) goto cleanup; - if (ret->models.models_len > REMOTE_CONNECT_CPU_MODELS_MAX) { + if (len > REMOTE_CONNECT_CPU_MODELS_MAX) { virReportError(VIR_ERR_RPC, _("Too many CPU models '%d' for limit '%d'"), - ret->models.models_len, REMOTE_CONNECT_CPU_MODELS_MAX); - virStringFreeList(models); + len, REMOTE_CONNECT_CPU_MODELS_MAX); goto cleanup; } - ret->models.models_val = models; - ret->models.models_len = len; + if (len && models) { + ret->models.models_val = models; + ret->models.models_len = len; + models = NULL; + } else { + ret->models.models_val = NULL; + ret->models.models_len = 0; + } + + ret->ret = len; rv = 0; cleanup: if (rv < 0) virNetMessageSaveError(rerr); + virStringFreeList(models); return rv; } diff --git i/src/remote/remote_driver.c w/src/remote/remote_driver.c index aa3762a..96ccb99 100644 --- i/src/remote/remote_driver.c +++ w/src/remote/remote_driver.c @@ -5548,51 +5548,57 @@ remoteConnectGetCPUModelNames(virConnectPtr conn, { int rv = -1; size_t i; - char **retmodels; + char **retmodels = NULL; remote_connect_get_cpu_model_names_args args; remote_connect_get_cpu_model_names_ret ret; + struct private_data *priv = conn->privateData; - remoteDriverLock(priv); - memset(&args, 0, sizeof(args)); - memset(&ret, 0, sizeof(ret)); + remoteDriverLock(priv); args.arch = (char *) arch; + args.need_results = !!models; args.flags = flags; + memset(&ret, 0, sizeof(ret)); if (call(conn, priv, 0, REMOTE_PROC_CONNECT_GET_CPU_MODEL_NAMES, (xdrproc_t) xdr_remote_connect_get_cpu_model_names_args, (char *) &args, (xdrproc_t) xdr_remote_connect_get_cpu_model_names_ret, (char *) &ret) < 0) - goto error; + goto done; /* Check the length of the returned list carefully. */ if (ret.models.models_len > REMOTE_CONNECT_CPU_MODELS_MAX) { - virReportError(VIR_ERR_RPC, "%s", - _("remoteConnectGetCPUModelNames: " - "returned number of CPU models exceeds limit")); - goto error; + virReportError(VIR_ERR_RPC, + _("Too many model names '%d' for limit '%d'"), + ret.models.models_len, + REMOTE_CONNECT_CPU_MODELS_MAX); + goto cleanup; } - if (VIR_ALLOC_N(retmodels, ret.models.models_len + 1) < 0) - goto error; + if (models) { + if (VIR_ALLOC_N(retmodels, ret.models.models_len + 1) < 0) + goto cleanup; - for (i = 0; i < ret.models.models_len; i++) - retmodels[i] = ret.models.models_val[i]; + for (i = 0; i < ret.models.models_len; i++) { + retmodels[i] = ret.models.models_val[i]; + ret.models.models_val[i] = NULL; + } + *models = retmodels; + retmodels = NULL; + } - /* Caller frees MODELS. */ - *models = retmodels; - rv = ret.models.models_len; + rv = ret.ret; + +cleanup: + virStringFreeList(retmodels); + + xdr_free((xdrproc_t) xdr_remote_connect_get_cpu_model_names_ret, (char *) &ret); done: remoteDriverUnlock(priv); return rv; - -error: - rv = -1; - virStringFreeList(ret.models.models_val); - goto done; } diff --git i/src/remote/remote_protocol.x w/src/remote/remote_protocol.x index bb8b0ce..8d17bad 100644 --- i/src/remote/remote_protocol.x +++ w/src/remote/remote_protocol.x @@ -2840,11 +2840,13 @@ struct remote_domain_event_device_removed_msg { struct remote_connect_get_cpu_model_names_args { remote_nonnull_string arch; + int need_results; unsigned int flags; }; struct remote_connect_get_cpu_model_names_ret { remote_nonnull_string models<REMOTE_CONNECT_CPU_MODELS_MAX>; + int ret; }; /*----- Protocol. -----*/ diff --git i/src/remote_protocol-structs w/src/remote_protocol-structs index 9148202..98d2d5b 100644 --- i/src/remote_protocol-structs +++ w/src/remote_protocol-structs @@ -2318,6 +2318,7 @@ struct remote_domain_event_device_removed_msg { }; struct remote_connect_get_cpu_model_names_args { remote_nonnull_string arch; + int need_results; u_int flags; }; struct remote_connect_get_cpu_model_names_ret { @@ -2325,6 +2326,7 @@ struct remote_connect_get_cpu_model_names_ret { u_int models_len; remote_nonnull_string * models_val; } models; + int ret; }; enum remote_procedure { REMOTE_PROC_CONNECT_OPEN = 1, -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/qemu/qemu_driver.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bbf2d23..c139daa 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15880,6 +15880,19 @@ qemuNodeSuspendForDuration(virConnectPtr conn, return nodeSuspendForDuration(target, duration, flags); } +static int +qemuConnectGetCPUModelNames(virConnectPtr conn, + const char *arch, + char ***models, + unsigned int flags) +{ + virCheckFlags(0, -1); + if (virConnectGetCPUModelNamesEnsureACL(conn) < 0) + return -1; + + return cpuGetModels(arch, models); +} + static virDriver qemuDriver = { .no = VIR_DRV_QEMU, @@ -16067,6 +16080,7 @@ static virDriver qemuDriver = { .domainMigratePerform3Params = qemuDomainMigratePerform3Params, /* 1.1.0 */ .domainMigrateFinish3Params = qemuDomainMigrateFinish3Params, /* 1.1.0 */ .domainMigrateConfirm3Params = qemuDomainMigrateConfirm3Params, /* 1.1.0 */ + .connectGetCPUModelNames = qemuConnectGetCPUModelNames, /* 1.1.3 */ }; -- 1.8.3.1
On 09/11/2013 08:13 AM, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/qemu/qemu_driver.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/test/test_driver.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index f9eee44..fb4c1d9 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -54,6 +54,7 @@ #include "virtypedparam.h" #include "virrandom.h" #include "virstring.h" +#include "cpu/cpu.h" #define VIR_FROM_THIS VIR_FROM_TEST @@ -5801,6 +5802,15 @@ testDomainScreenshot(virDomainPtr dom ATTRIBUTE_UNUSED, return ret; } +static int +testConnectGetCPUModelNames(virConnectPtr conn ATTRIBUTE_UNUSED, + const char *arch, + char ***models, + unsigned int flags) +{ + virCheckFlags(0, -1); + return cpuGetModels(arch, models); +} static virDriver testDriver = { .no = VIR_DRV_TEST, @@ -5872,6 +5882,7 @@ static virDriver testDriver = { .connectIsAlive = testConnectIsAlive, /* 0.9.8 */ .nodeGetCPUMap = testNodeGetCPUMap, /* 1.0.0 */ .domainScreenshot = testDomainScreenshot, /* 1.0.5 */ + .connectGetCPUModelNames = testConnectGetCPUModelNames, /* 1.1.3 */ }; static virNetworkDriver testNetworkDriver = { -- 1.8.3.1
On 09/11/2013 08:13 AM, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/test/test_driver.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- tools/virsh-host.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 5 +++++ 2 files changed, 59 insertions(+) diff --git a/tools/virsh-host.c b/tools/virsh-host.c index 880ae4b..5d47428 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -627,6 +627,54 @@ cmdURI(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) } /* + * "cpu-models" command + */ +static const vshCmdInfo info_cpu_models[] = { + {.name = "help", + .data = N_("CPU models") + }, + {.name = "desc", + .data = N_("Get the CPU models for an arch.") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_cpu_models[] = { + {.name = "arch", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("architecture") + }, + {.name = NULL} +}; + +static bool +cmdCPUModelNames(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) +{ + char **models; + size_t i; + int nmodels; + const char *arch = NULL; + + if (vshCommandOptStringReq(ctl, cmd, "arch", &arch) < 0) + return false; + + nmodels = virConnectGetCPUModelNames(ctl->conn, arch, &models, 0); + if (nmodels < 0) { + vshError(ctl, "%s", _("failed to get CPU model names")); + return false; + } + + for (i = 0; i < nmodels; i++) { + vshPrint(ctl, "%s\n", models[i]); + VIR_FREE(models[i]); + } + VIR_FREE(models); + + return true; +} + +/* * "version" command */ static const vshCmdInfo info_version[] = { @@ -851,6 +899,12 @@ const vshCmdDef hostAndHypervisorCmds[] = { .info = info_capabilities, .flags = 0 }, + {.name = "cpu-models", + .handler = cmdCPUModelNames, + .opts = opts_cpu_models, + .info = info_cpu_models, + .flags = 0 + }, {.name = "freecell", .handler = cmdFreecell, .opts = opts_freecell, diff --git a/tools/virsh.pod b/tools/virsh.pod index 0ae5178..72555e7 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -163,6 +163,7 @@ group as an option. For example: Host and Hypervisor (help keyword 'host'): capabilities capabilities + cpu-models show the CPU models for an architecture connect (re)connect to hypervisor freecell NUMA free memory hostname print the hypervisor hostname @@ -358,6 +359,10 @@ current domain is in. =over 4 +=item B<cpu-models> I<arch> + +Print the list of CPU models known for the specified architecture. + =item B<running> The domain is currently running on a CPU -- 1.8.3.1
On 09/11/2013 08:13 AM, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- tools/virsh-host.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 5 +++++ 2 files changed, 59 insertions(+)
+ +static bool +cmdCPUModelNames(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) +{ + char **models; + size_t i; + int nmodels; + const char *arch = NULL; + + if (vshCommandOptStringReq(ctl, cmd, "arch", &arch) < 0)
ATTRIBUTE_UNUSED is a lie - we just used it :)
+ return false; + + nmodels = virConnectGetCPUModelNames(ctl->conn, arch, &models, 0);
I tweaked this some during my testing of NULL models; but don't think we need to make virsh expose NULL models to the command line, so I'm not changing this. ACK with this squashed in (oh, I just noticed my emacs' copyright-checker kicked in for my edits; I wonder how many other files you touched that could use an updated copyright): diff --git i/tools/virsh-host.c w/tools/virsh-host.c index 5d47428..30940ad 100644 --- i/tools/virsh-host.c +++ w/tools/virsh-host.c @@ -1,7 +1,7 @@ /* * virsh-host.c: Commands in "Host and Hypervisor" group. * - * Copyright (C) 2005, 2007-2012 Red Hat, Inc. + * Copyright (C) 2005, 2007-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -649,7 +649,7 @@ static const vshCmdOptDef opts_cpu_models[] = { }; static bool -cmdCPUModelNames(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) +cmdCPUModelNames(vshControl *ctl, const vshCmd *cmd) { char **models; size_t i; -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/13/2013 02:58 PM, Eric Blake wrote:
On 09/11/2013 08:13 AM, Giuseppe Scrivano wrote:
I tweaked this some during my testing of NULL models; but don't think we need to make virsh expose NULL models to the command line, so I'm not changing this.
ACK with this squashed in (oh, I just noticed my emacs' copyright-checker kicked in for my edits; I wonder how many other files you touched that could use an updated copyright):
Blah. I get this far, where things are finally testable, and see: # tools/virsh cpu-models x86_64 2013-09-13 21:04:43.332+0000: 16843: error : cpuMapLoad:121 : internal error: cannot find CPU map for x86_64 architecture error: failed to get CPU model names error: internal error: cannot find CPU map for x86_64 architecture I'm now debugging whether it was one of the patches I modified, or a flaw in your original patch. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/13/2013 03:06 PM, Eric Blake wrote:
On 09/13/2013 02:58 PM, Eric Blake wrote:
On 09/11/2013 08:13 AM, Giuseppe Scrivano wrote:
I tweaked this some during my testing of NULL models; but don't think we need to make virsh expose NULL models to the command line, so I'm not changing this.
ACK with this squashed in (oh, I just noticed my emacs' copyright-checker kicked in for my edits; I wonder how many other files you touched that could use an updated copyright):
Blah. I get this far, where things are finally testable, and see:
# tools/virsh cpu-models x86_64 2013-09-13 21:04:43.332+0000: 16843: error : cpuMapLoad:121 : internal error: cannot find CPU map for x86_64 architecture error: failed to get CPU model names error: internal error: cannot find CPU map for x86_64 architecture
I'm now debugging whether it was one of the patches I modified, or a flaw in your original patch.
Jiri helped me on IRC. The list of arch names in 'virsh capabilities' is from a canonical list; but the list of arch names in /usr/share/libvirt/cpu_map.xml is a list of drivers (which can support one _or more than one_ canonical architecture). More precisely: $ grep 'arch ' /usr/share/libvirt/cpu_map.xml <arch name='x86'> <arch name='ppc64'> $ tools/virsh -c test:///default capabilities | grep 'arch ' <arch name='i686'> <arch name='i686'> this suggests that the 'x86' driver knows how to manage both the 'i686' and 'x86_64' architectures. So ideally, qemu-system-x86_64 should support 'virsh cpu-models x86_64' and 'virsh cpu-models i686' with identical lists (after all, x86_64 chips can run in i686 mode), while the test driver (which claims support for ONLY i686) might want to fail for x86_64. Furthermore, with your patches as-is, 'virsh cpu-models x86' returned a list; but I don't think this is appropriate as that name is not in the capabilities output. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, Sep 13, 2013 at 03:45:58PM -0600, Eric Blake wrote:
On 09/13/2013 03:06 PM, Eric Blake wrote:
On 09/13/2013 02:58 PM, Eric Blake wrote:
On 09/11/2013 08:13 AM, Giuseppe Scrivano wrote:
I tweaked this some during my testing of NULL models; but don't think we need to make virsh expose NULL models to the command line, so I'm not changing this.
ACK with this squashed in (oh, I just noticed my emacs' copyright-checker kicked in for my edits; I wonder how many other files you touched that could use an updated copyright):
Blah. I get this far, where things are finally testable, and see:
# tools/virsh cpu-models x86_64 2013-09-13 21:04:43.332+0000: 16843: error : cpuMapLoad:121 : internal error: cannot find CPU map for x86_64 architecture error: failed to get CPU model names error: internal error: cannot find CPU map for x86_64 architecture
I'm now debugging whether it was one of the patches I modified, or a flaw in your original patch.
Jiri helped me on IRC. The list of arch names in 'virsh capabilities' is from a canonical list; but the list of arch names in /usr/share/libvirt/cpu_map.xml is a list of drivers (which can support one _or more than one_ canonical architecture). More precisely:
$ grep 'arch ' /usr/share/libvirt/cpu_map.xml <arch name='x86'> <arch name='ppc64'> $ tools/virsh -c test:///default capabilities | grep 'arch ' <arch name='i686'> <arch name='i686'>
this suggests that the 'x86' driver knows how to manage both the 'i686' and 'x86_64' architectures. So ideally, qemu-system-x86_64 should support 'virsh cpu-models x86_64' and 'virsh cpu-models i686' with identical lists (after all, x86_64 chips can run in i686 mode), while the test driver (which claims support for ONLY i686) might want to fail for x86_64. Furthermore, with your patches as-is, 'virsh cpu-models x86' returned a list; but I don't think this is appropriate as that name is not in the capabilities output.
Agreed, that is bad. We need to have the API accepting formal arch names, as defined by virArch enum only, and translate to the names used by the CPU database. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 09/11/2013 08:13 AM, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- tools/virsh-host.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 5 +++++ 2 files changed, 59 insertions(+)
+ +static const vshCmdOptDef opts_cpu_models[] = { + {.name = "arch", + .type = VSH_OT_DATA, + .flags = VSH_OFLAG_REQ, + .help = N_("architecture") + },
Question: would it make sense to have --arch be optional, and to default it to the arch learned by uname() if not present? I guess for local systems, that's a convenience factor; for remote systems, it's not quite the right thing (using virsh on a ppc box to manage a libvirtd on an x86_64 box for example). Or maybe if --arch is optional, we call GetCapabilities under the hood, and enumerate over ALL possible arches scraped from the capabilities (of course, with slightly different output layout to distinguish which model names correspond to which arches)? That could be a followup patch, though. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- python/generator.py | 2 +- python/libvirt-override-api.xml | 7 ++++++ python/libvirt-override.c | 52 +++++++++++++++++++++++++++++++++++++++++ python/libvirt-override.py | 11 +++++++++ 4 files changed, 71 insertions(+), 1 deletion(-) diff --git a/python/generator.py b/python/generator.py index 73107d7..12c14f1 100755 --- a/python/generator.py +++ b/python/generator.py @@ -250,6 +250,7 @@ lxc_functions_failed = [] qemu_functions_failed = [] functions_skipped = [ "virConnectListDomains", + "virConnectGetCPUModelNames", ] lxc_functions_skipped = [] qemu_functions_skipped = [] @@ -366,7 +367,6 @@ foreign_encoding_args = ( # Class methods which are written by hand in libvirt.c but the Python-level # code is still automatically generated (so they are not in skip_function()). skip_impl = ( - "virConnectGetCPUModelNames", 'virConnectGetVersion', 'virConnectGetLibVersion', 'virConnectListDomainsID', diff --git a/python/libvirt-override-api.xml b/python/libvirt-override-api.xml index 9a88215..1bceb05 100644 --- a/python/libvirt-override-api.xml +++ b/python/libvirt-override-api.xml @@ -476,6 +476,13 @@ <arg name='xmlCPUs' type='const char **' info='array of XML descriptions of host CPUs'/> <arg name='flags' type='unsigned int' info='fine-tuning flags, currently unused, pass 0.'/> </function> + <function name='virConnectGetCPUModelNames' file='python'> + <info>Get the list of supported CPU models.</info> + <return type='char *' info='list of supported CPU models'/> + <arg name='conn' type='virConnectPtr' info='virConnect connection'/> + <arg name='arch' type='const char *' info='arch'/> + <arg name='flags' type='unsigned int' info='fine-tuning flags, currently unused, pass 0.'/> + </function> <function name='virDomainSnapshotListNames' file='python'> <info>collect the list of snapshot names for the given domain</info> <arg name='dom' type='virDomainPtr' info='pointer to the domain'/> diff --git a/python/libvirt-override.c b/python/libvirt-override.c index d16b9a2..63c2566 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -2276,6 +2276,58 @@ libvirt_virConnectGetVersion(PyObject *self ATTRIBUTE_UNUSED, return PyInt_FromLong(hvVersion); } +PyObject * +libvirt_virConnectGetCPUModelNames(PyObject *self ATTRIBUTE_UNUSED, + PyObject *args) +{ + int c_retval; + virConnectPtr conn; + PyObject *rv = NULL, *pyobj_conn; + char **models = NULL; + size_t i; + int flags = 0; + const char *arch = NULL; + + if (!PyArg_ParseTuple(args, (char *)"Osi:virConnectGetCPUModelNames", + &pyobj_conn, &arch, &flags)) + return NULL; + conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn); + + LIBVIRT_BEGIN_ALLOW_THREADS; + + c_retval = virConnectGetCPUModelNames(conn, arch, &models, flags); + + LIBVIRT_END_ALLOW_THREADS; + + if (c_retval == -1) + return VIR_PY_INT_FAIL; + + if ((rv = PyList_New(c_retval)) == NULL) + goto error; + + for (i = 0; i < c_retval; i++) { + PyObject *str; + if ((str = PyString_FromString(models[i])) == NULL) + goto error; + + PyList_SET_ITEM(rv, i, str); + } + +done: + if (models) { + for (i = 0; i < c_retval; i++) + VIR_FREE(models[i]); + VIR_FREE(models); + } + + return rv; + +error: + Py_XDECREF(rv); + rv = VIR_PY_INT_FAIL; + goto done; +} + static PyObject * libvirt_virConnectGetLibVersion(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) diff --git a/python/libvirt-override.py b/python/libvirt-override.py index ccfec48..3471a43 100644 --- a/python/libvirt-override.py +++ b/python/libvirt-override.py @@ -207,3 +207,14 @@ def virEventAddTimeout(timeout, cb, opaque): ret = libvirtmod.virEventAddTimeout(timeout, cbData) if ret == -1: raise libvirtError ('virEventAddTimeout() failed') return ret + +def getCPUModelNames(conn, arch, flags=0): + """ + get the list of supported CPU models. + @conn: virConnect connection + @arch: Architecture + @flags: extra flags; not used yet, so callers should always pass 0. + """ + ret = libvirtmod.virConnectGetCPUModelNames(conn._o, arch, flags) + if ret == None: raise libvirtError ('virConnectGetCPUModelNames() failed', conn=self) + return ret -- 1.8.3.1
On 09/11/2013 08:13 AM, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- python/generator.py | 2 +- python/libvirt-override-api.xml | 7 ++++++ python/libvirt-override.c | 52 +++++++++++++++++++++++++++++++++++++++++ python/libvirt-override.py | 11 +++++++++ 4 files changed, 71 insertions(+), 1 deletion(-)
+ + LIBVIRT_BEGIN_ALLOW_THREADS; + + c_retval = virConnectGetCPUModelNames(conn, arch, &models, flags); + + LIBVIRT_END_ALLOW_THREADS; + + if (c_retval == -1) + return VIR_PY_INT_FAIL; + + if ((rv = PyList_New(c_retval)) == NULL) + goto error; + + for (i = 0; i < c_retval; i++) { + PyObject *str; + if ((str = PyString_FromString(models[i])) == NULL) + goto error; + + PyList_SET_ITEM(rv, i, str);
Elsewhere, we've used PyList_New(0)/PyList_Append() rather than PyList_New(count)/PyList_SET_ITEM(); but that's not universal; also, I see uses of PyList_SetItem but not PyList_SET_ITEM; what's the difference? More importantly, you STILL have a data leak. If c_retval is 2 but PyString_FromString(models[1]) fails (typically only possible on OOM), then we goto error and never free models. Maybe you can get some inspiration from libvirt_virDomainSnapshotListNames for how to properly have a single cleanup: label (instead of done:/error:), while handling error cases and setting up proper transfer semantics from an array of strings to a python list of strings. I'm looking forward to v4; the changes I suggested squashing in have all worked in my testing, and it is merely this patch and patch 2 that need actual fixes that I have not written. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Eric Blake <eblake@redhat.com> writes:
On 09/11/2013 08:13 AM, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- python/generator.py | 2 +- python/libvirt-override-api.xml | 7 ++++++ python/libvirt-override.c | 52 +++++++++++++++++++++++++++++++++++++++++ python/libvirt-override.py | 11 +++++++++ 4 files changed, 71 insertions(+), 1 deletion(-)
+ + LIBVIRT_BEGIN_ALLOW_THREADS; + + c_retval = virConnectGetCPUModelNames(conn, arch, &models, flags); + + LIBVIRT_END_ALLOW_THREADS; + + if (c_retval == -1) + return VIR_PY_INT_FAIL; + + if ((rv = PyList_New(c_retval)) == NULL) + goto error; + + for (i = 0; i < c_retval; i++) { + PyObject *str; + if ((str = PyString_FromString(models[i])) == NULL) + goto error; + + PyList_SET_ITEM(rv, i, str);
Elsewhere, we've used PyList_New(0)/PyList_Append() rather than PyList_New(count)/PyList_SET_ITEM(); but that's not universal; also, I see uses of PyList_SetItem but not PyList_SET_ITEM; what's the difference?
PyList_SET_ITEM is just an optimized version of PyList_SetItem that doesn't do any error checking or attempt to release an object already present in the slot. It is ok to use here, as we are sure the rv list is free when we fill it with model names. I think there are other cases in libvirt-override.c where we can replace PyList_SetItem with PyList_SET_ITEM without any harm. Giuseppe
On 09/11/2013 08:13 AM, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- python/generator.py | 2 +- python/libvirt-override-api.xml | 7 ++++++ python/libvirt-override.c | 52 +++++++++++++++++++++++++++++++++++++++++ python/libvirt-override.py | 11 +++++++++ 4 files changed, 71 insertions(+), 1 deletion(-)
+ goto error; + + PyList_SET_ITEM(rv, i, str); + } + +done: + if (models) { + for (i = 0; i < c_retval; i++) + VIR_FREE(models[i]); + VIR_FREE(models); + } + + return rv; + +error: + Py_XDECREF(rv); + rv = VIR_PY_INT_FAIL; + goto done;
Oh bother - time for me to call it a weekend. I thought you were _returning_ VIR_PY_INT_FAIL here without touching models, but you are correctly jumping back to 'done' to get the cleanup. Looks like you don't have a leak after all. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Giuseppe Scrivano -
Jiri Denemark