[libvirt] [PATCH v2 0/5] 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). *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 (5): cpu_models: add new public API cpu_models: implement the remote protocol cpu_models: add the support for qemu cpu_models: add the support for the test protocol cpu_models: add Python bindings daemon/remote.c | 47 ++++++++++++++++++++++++++++++ include/libvirt/libvirt.h.in | 18 ++++++++++++ python/generator.py | 1 + python/libvirt-override-api.xml | 7 +++++ python/libvirt-override.c | 56 ++++++++++++++++++++++++++++++++++++ python/libvirt-override.py | 11 +++++++ src/cpu/cpu.c | 64 +++++++++++++++++++++++++++++++++++++++++ src/cpu/cpu.h | 3 ++ src/driver.h | 7 +++++ src/libvirt.c | 47 ++++++++++++++++++++++++++++++ src/libvirt_private.syms | 1 + src/libvirt_public.syms | 5 ++++ src/qemu/qemu_driver.c | 14 +++++++++ src/remote/remote_driver.c | 59 +++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 20 ++++++++++++- src/remote_protocol-structs | 11 +++++++ src/test/test_driver.c | 16 +++++++++++ tools/virsh-host.c | 48 +++++++++++++++++++++++++++++++ tools/virsh.pod | 5 ++++ 19 files changed, 439 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 | 18 +++++++++++++ python/generator.py | 1 + src/cpu/cpu.c | 64 ++++++++++++++++++++++++++++++++++++++++++++ src/cpu/cpu.h | 3 +++ src/driver.h | 7 +++++ src/libvirt.c | 47 ++++++++++++++++++++++++++++++++ src/libvirt_private.syms | 1 + src/libvirt_public.syms | 5 ++++ tools/virsh-host.c | 48 +++++++++++++++++++++++++++++++++ tools/virsh.pod | 5 ++++ 10 files changed, 199 insertions(+) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index a47e33c..43fb738 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -4006,6 +4006,24 @@ int virConnectCompareCPU(virConnectPtr conn, const char *xmlDesc, unsigned int flags); +/** + * 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, 0 on success. + */ +int virConnectGetCPUModelNames(virConnectPtr conn, + const char *arch, + char ***models, + unsigned int flags); /** * virConnectBaselineCPUFlags diff --git a/python/generator.py b/python/generator.py index fb321c6..0301403 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/cpu/cpu.c b/src/cpu/cpu.c index 50c2de3..854c272 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -27,6 +27,7 @@ #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" @@ -456,3 +457,66 @@ 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; + + if (VIR_EXPAND_N(data->data, data->len, 1) < 0) + return -1; + + data->data[data->len - 2] = name; + data->data[data->len - 1] = NULL; + return 0; +} + + +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 0; + +error: + if (data.data) { + char **it; + for (it = data.data; *it; it++) + VIR_FREE(*it); + VIR_FREE(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/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 07a3fd5..f75469f 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -18519,6 +18519,53 @@ 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, 0 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; + } + + if (arch == NULL) { + virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + + if (conn->driver->connectGetCPUModelNames) { + if (conn->driver->connectGetCPUModelNames(conn, arch, models, flags) < 0) + goto error; + + return 0; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(conn); + return -1; +} + + +/** * virConnectBaselineCPU: * * @conn: virConnect connection 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; 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 .... diff --git a/tools/virsh-host.c b/tools/virsh-host.c index 880ae4b..ebab5ae 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -92,6 +92,25 @@ static const vshCmdOptDef opts_freecell[] = { {.name = NULL} }; +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 cmdFreecell(vshControl *ctl, const vshCmd *cmd) { @@ -626,6 +645,29 @@ cmdURI(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) return true; } +static bool +cmdCPUModelNames(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) +{ + char **models, **it; + const char *arch = NULL; + + if (vshCommandOptStringReq(ctl, cmd, "arch", &arch) < 0) + return false; + + if (virConnectGetCPUModelNames(ctl->conn, arch, &models, 0) < 0) { + vshError(ctl, "%s", _("failed to get CPU Models Names")); + return false; + } + + for (it = models; *it; it++) { + vshPrint(ctl, "%s\n", *it); + VIR_FREE(*it); + } + VIR_FREE(models); + + return true; +} + /* * "version" command */ @@ -851,6 +893,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 Sat, Sep 07, 2013 at 01:11:22AM +0200, 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> --- include/libvirt/libvirt.h.in | 18 +++++++++++++ python/generator.py | 1 + src/cpu/cpu.c | 64 ++++++++++++++++++++++++++++++++++++++++++++ src/cpu/cpu.h | 3 +++ src/driver.h | 7 +++++ src/libvirt.c | 47 ++++++++++++++++++++++++++++++++ src/libvirt_private.syms | 1 + src/libvirt_public.syms | 5 ++++ tools/virsh-host.c | 48 +++++++++++++++++++++++++++++++++ tools/virsh.pod | 5 ++++
It is preferrable to have virsh changes separate from the public API addition. Likewise I'd suggest th src/cpu/ changes be a separate patch.
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index a47e33c..43fb738 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -4006,6 +4006,24 @@ int virConnectCompareCPU(virConnectPtr conn, const char *xmlDesc, unsigned int flags);
+/** + * 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, 0 on success.
I'd suggest Returns -1 on error, number of elements in @models on success
+ */ +int virConnectGetCPUModelNames(virConnectPtr conn, + const char *arch, + char ***models, + unsigned int flags);
+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 0; + +error: + if (data.data) { + char **it; + for (it = data.data; *it; it++) + VIR_FREE(*it); + VIR_FREE(data.data);
virFreeStringList(data.data); should do the trick.
/** + * 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, 0 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; + } + + if (arch == NULL) { + virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + }
This allows access to this API from readonly connections. I think this is ok, but just wanted to mention it explicitly.
+ + if (conn->driver->connectGetCPUModelNames) { + if (conn->driver->connectGetCPUModelNames(conn, arch, models, flags) < 0) + goto error; + + return 0; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(conn); + return -1; +} + + +/** * virConnectBaselineCPU: * * @conn: virConnect connection
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/10/2013 08:38 AM, Daniel P. Berrange wrote:
On Sat, Sep 07, 2013 at 01:11:22AM +0200, 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> --- include/libvirt/libvirt.h.in | 18 +++++++++++++ python/generator.py | 1 + src/cpu/cpu.c | 64 ++++++++++++++++++++++++++++++++++++++++++++ src/cpu/cpu.h | 3 +++ src/driver.h | 7 +++++ src/libvirt.c | 47 ++++++++++++++++++++++++++++++++ src/libvirt_private.syms | 1 + src/libvirt_public.syms | 5 ++++ tools/virsh-host.c | 48 +++++++++++++++++++++++++++++++++ tools/virsh.pod | 5 ++++
It is preferrable to have virsh changes separate from the public API addition. Likewise I'd suggest th src/cpu/ changes be a separate patch.
I can go either way regarding virsh + libvirt.c - virsh changes at the same time as the public API prove that the public API is usable from a coding perspective. Git history says we've done both approaches in the past. But I totally agree that libvirt.c + src/cpu should be separate; committing the API first and then adding the implementation makes for a nice division. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/06/2013 05:11 PM, 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> --- include/libvirt/libvirt.h.in | 18 +++++++++++++ python/generator.py | 1 + src/cpu/cpu.c | 64 ++++++++++++++++++++++++++++++++++++++++++++ src/cpu/cpu.h | 3 +++ src/driver.h | 7 +++++ src/libvirt.c | 47 ++++++++++++++++++++++++++++++++ src/libvirt_private.syms | 1 + src/libvirt_public.syms | 5 ++++ tools/virsh-host.c | 48 +++++++++++++++++++++++++++++++++ tools/virsh.pod | 5 ++++ 10 files changed, 199 insertions(+)
In addition to Daniel's review,
+++ b/include/libvirt/libvirt.h.in @@ -4006,6 +4006,24 @@ int virConnectCompareCPU(virConnectPtr conn, const char *xmlDesc, unsigned int flags);
+/** + * 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, 0 on success. + */ +int virConnectGetCPUModelNames(virConnectPtr conn, + const char *arch, + char ***models, + unsigned int flags);
Typically we have not documented functions in the .h, just in src/libvirt.c.
+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; + + if (VIR_EXPAND_N(data->data, data->len, 1) < 0) + return -1; + + data->data[data->len - 2] = name; + data->data[data->len - 1] = NULL; + return 0;
VIR_INSERT_ELEMENT may be simpler to use than VIR_EXPAND_N. Furthermore, VIR_EXPAND_N guarantees zero-initialization of the growth, so the data->data[data->len - 1] = NULL; line is redundant.
+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; + } + + if (arch == NULL) { + virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + }
virCheckNonNullArgReturn(arch, -1)
+ + if (conn->driver->connectGetCPUModelNames) { + if (conn->driver->connectGetCPUModelNames(conn, arch, models, flags) < 0) + goto error; + + return 0;
I agree with Dan that this should return the number of non-NULL entries (so setting the array to { "x86_64", "i686", NULL } would return 2, even though the array has allocated room for [at least] 3 pointers).
+++ b/tools/virsh-host.c @@ -92,6 +92,25 @@ static const vshCmdOptDef opts_freecell[] = { {.name = NULL} };
+static const vshCmdInfo info_cpu_models[] = { + {.name = "help", + .data = N_("CPU models.")
We generally don't have trailing '.' in the short help.
+ }, + {.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} +}; +
These new [] data should be closer...
static bool cmdFreecell(vshControl *ctl, const vshCmd *cmd) { @@ -626,6 +645,29 @@ cmdURI(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) return true; }
+static bool +cmdCPUModelNames(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
...to the function they are associated with. (That is, don't split cmdFreecell from its options).
+{ + char **models, **it; + const char *arch = NULL; + + if (vshCommandOptStringReq(ctl, cmd, "arch", &arch) < 0) + return false; + + if (virConnectGetCPUModelNames(ctl->conn, arch, &models, 0) < 0) { + vshError(ctl, "%s", _("failed to get CPU Models Names"));
Sounds awkward; maybe "failed to get CPU model names"
@@ -851,6 +893,12 @@ const vshCmdDef hostAndHypervisorCmds[] = { .info = info_capabilities, .flags = 0 }, + {.name = "cpu-models", + .handler = cmdCPUModelNames, + .opts = opts_cpu_models, + .info = info_cpu_models, + .flags = 0
Not your fault, but I'd like trailing commas in all of these initializers (as a separate cleanup), so that future additions can be strict additions (thinking forward to Tomas' work on tab-completers). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- daemon/remote.c | 47 +++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 59 ++++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 20 ++++++++++++++- src/remote_protocol-structs | 11 +++++++++ 4 files changed, 136 insertions(+), 1 deletion(-) diff --git a/daemon/remote.c b/daemon/remote.c index 2aff7c1..4846ccb 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5037,6 +5037,53 @@ 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 rv = -1; + char **models; + struct daemonClientPrivate *priv = + virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (virConnectGetCPUModelNames(priv->conn, args->arch, &models, + args->flags) < 0) + goto cleanup; + + ret->models.models_val = models; + ret->models.models_len = 0; + while (*models++) + ret->models.models_len++; + + 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); + + for (models = ret->models.models_val; *models; models++) + VIR_FREE(*models); + VIR_FREE(ret->models.models_val); + goto cleanup; + } + + 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..1b6635f 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -5541,6 +5541,64 @@ 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 = 0; + +done: + remoteDriverUnlock(priv); + return rv; + +error: + rv = -1; + for (i = 0; i < ret.models.models_len; i++) + VIR_FREE(ret.models.models_val[i]); + VIR_FREE(ret.models.models_val); + goto done; +} + + +static int remoteDomainOpenGraphics(virDomainPtr dom, unsigned int idx, int fd, @@ -6933,6 +6991,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 Sat, Sep 07, 2013 at 01:11:23AM +0200, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- daemon/remote.c | 47 +++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 59 ++++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 20 ++++++++++++++- src/remote_protocol-structs | 11 +++++++++ 4 files changed, 136 insertions(+), 1 deletion(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 2aff7c1..4846ccb 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -5037,6 +5037,53 @@ 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 rv = -1; + char **models; + struct daemonClientPrivate *priv = + virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (virConnectGetCPUModelNames(priv->conn, args->arch, &models, + args->flags) < 0) + goto cleanup; + + ret->models.models_val = models; + ret->models.models_len = 0; + while (*models++) + ret->models.models_len++;
You can avoid this if you make the API return value indicate the number of models.
+ + 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); + + for (models = ret->models.models_val; *models; models++) + VIR_FREE(*models); + VIR_FREE(ret->models.models_val);
virStringFeeList(models)
+ goto cleanup; + } + + 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..1b6635f 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -5541,6 +5541,64 @@ 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 = 0; + +done: + remoteDriverUnlock(priv); + return rv; + +error: + rv = -1; + for (i = 0; i < ret.models.models_len; i++) + VIR_FREE(ret.models.models_val[i]); + VIR_FREE(ret.models.models_val);
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/06/2013 05:11 PM, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- daemon/remote.c | 47 +++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 59 ++++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 20 ++++++++++++++- src/remote_protocol-structs | 11 +++++++++ 4 files changed, 136 insertions(+), 1 deletion(-)
In addition to Dan's comments:
+++ b/src/remote/remote_driver.c
+error: + rv = -1; + for (i = 0; i < ret.models.models_len; i++) + VIR_FREE(ret.models.models_val[i]); + VIR_FREE(ret.models.models_val);
Another virStringFreeList() -- 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 e37fe33..831a296 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15882,6 +15882,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, @@ -16069,6 +16082,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/06/2013 05:11 PM, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/qemu/qemu_driver.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
ACK. Would any of the drivers besides qemu and test benefit from an implementation? For example, does this work for libxl? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Sep 10, 2013 at 05:55:57PM -0600, Eric Blake wrote:
On 09/06/2013 05:11 PM, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/qemu/qemu_driver.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
ACK. Would any of the drivers besides qemu and test benefit from an implementation? For example, does this work for libxl?
Only the QEMU driver has CPU model support wired up. 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 :|
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/test/test_driver.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index c225618..03d9c96 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5801,6 +5801,21 @@ testDomainScreenshot(virDomainPtr dom ATTRIBUTE_UNUSED, return ret; } +static int +testConnectGetCPUModelNames(virConnectPtr conn ATTRIBUTE_UNUSED, + const char *arch ATTRIBUTE_UNUSED, + char ***models, + unsigned int flags) +{ + char **null_list; + + virCheckFlags(0, -1); + if (VIR_ALLOC_N(null_list, 1) < 0) + return -1; + + *models = null_list; + return 0; +} static virDriver testDriver = { .no = VIR_DRV_TEST, @@ -5872,6 +5887,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 Sat, Sep 07, 2013 at 01:11:25AM +0200, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- src/test/test_driver.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index c225618..03d9c96 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5801,6 +5801,21 @@ testDomainScreenshot(virDomainPtr dom ATTRIBUTE_UNUSED, return ret; }
+static int +testConnectGetCPUModelNames(virConnectPtr conn ATTRIBUTE_UNUSED, + const char *arch ATTRIBUTE_UNUSED, + char ***models, + unsigned int flags) +{ + char **null_list; + + virCheckFlags(0, -1); + if (VIR_ALLOC_N(null_list, 1) < 0) + return -1; + + *models = null_list; + return 0; +}
It'd be nice to have a non-NULL list for this IMHO. 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 :|
"Daniel P. Berrange" <berrange@redhat.com> writes:
+static int +testConnectGetCPUModelNames(virConnectPtr conn ATTRIBUTE_UNUSED, + const char *arch ATTRIBUTE_UNUSED, + char ***models, + unsigned int flags) +{ + char **null_list; + + virCheckFlags(0, -1); + if (VIR_ALLOC_N(null_list, 1) < 0) + return -1; + + *models = null_list; + return 0; +}
It'd be nice to have a non-NULL list for this IMHO.
I'll use cpuGetModels here as well. It shouldn't be a problem to use the same data as the qemu driver. Regards, Giuseppe
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> --- python/generator.py | 2 +- python/libvirt-override-api.xml | 7 ++++++ python/libvirt-override.c | 56 +++++++++++++++++++++++++++++++++++++++++ python/libvirt-override.py | 11 ++++++++ 4 files changed, 75 insertions(+), 1 deletion(-) diff --git a/python/generator.py b/python/generator.py index 0301403..ba2cc3e 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..8536561 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -2276,6 +2276,62 @@ 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, *pyobj_conn; + char **it, **models = NULL; + int flags = 0; + const char *arch = NULL; + unsigned int len; + + 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; + + len = 0; + for (it = models; *it; it++) + len++; + + if ((rv = PyList_New(len)) == NULL) + goto error; + + len = 0; + for (it = models; *it; it++){ + PyObject *str; + if ((str = PyString_FromString(*it)) == NULL) + goto error; + + PyList_SET_ITEM(rv, len++, str); + } + +done: + if (models) { + for (it = models; *it; it++) + VIR_FREE(*it); + VIR_FREE(models); + } + + return rv; + +error: + 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/06/2013 05:11 PM, 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 | 56 +++++++++++++++++++++++++++++++++++++++++ python/libvirt-override.py | 11 ++++++++ 4 files changed, 75 insertions(+), 1 deletion(-)
+ + len = 0; + for (it = models; *it; it++) + len++; + + if ((rv = PyList_New(len)) == NULL) + goto error; +
Again, the C return value should make it so you don't have to re-count len via 'it' yourself.
+ len = 0; + for (it = models; *it; it++){
Space before {
+ PyObject *str; + if ((str = PyString_FromString(*it)) == NULL) + goto error; + + PyList_SET_ITEM(rv, len++, str); + } + +done: + if (models) { + for (it = models; *it; it++) + VIR_FREE(*it); + VIR_FREE(models); + } + + return rv; + +error: + rv = VIR_PY_INT_FAIL;
Ouch. Memory leak of the former value of rv. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Giuseppe Scrivano