[libvirt] [PATCH] xen: Use internal interfaces in xenDomainUsedCpus
Jim Fehlig
jfehlig at suse.com
Mon Aug 5 18:52:47 UTC 2013
Hi Stefan,
Sorry for missing this patch and thanks for the reminder poke...
Stefan Bader wrote:
> From 0df7a75cb777c2f616d5d0e16a6eb8b3db4b1f15 Mon Sep 17 00:00:00 2001
> From: Stefan Bader <stefan.bader at canonical.com>
> Date: Mon, 15 Jul 2013 16:03:58 +0200
> Subject: [PATCH] xen: Use internal interfaces in xenDomainUsedCpus
>
> Since commit 95e18efd most public interfaces (xenUnified...) obtain
> a virDomainDefPtr via xenGetDomainDefFor...() which take the unified
> lock.
> This is already taken before calling xenDomainUsedCpus(), so we get
> a deadlock for active guests. Avoid this by splitting up
> xenUnifiedDomainGetVcpusFlags() and xenUnifiedDomainGetVcpus() into
> public and private function calls (which get the virDomainDefPtr passed)
> and use those in xenDomainUsedCpus().
>
> xenDomainUsedCpus
> ...
> nb_vcpu = xenUnifiedDomainGetMaxVcpus(dom);
> return xenUnifiedDomainGetVcpusFlags(...)
> ...
> if (!(def = xenGetDomainDefForDom(dom)))
> return xenGetDomainDefForUUID(dom->conn, dom->uuid);
> ...
> ret = xenHypervisorLookupDomainByUUID(conn, uuid);
> ...
> xenUnifiedLock(priv);
> name = xenStoreDomainGetName(conn, id);
> xenUnifiedUnlock(priv);
> ...
> if ((ncpus = xenUnifiedDomainGetVcpus(dom, cpuinfo, nb_vcpu,
> ...
> if (!(def = xenGetDomainDefForDom(dom)))
> [again like above]
>
> Signed-off-by: Stefan Bader <stefan.bader at canonical.com>
> ---
> src/xen/xen_driver.c | 94 +++++++++++++++++++++++++++++++++-----------------
> src/xen/xen_driver.h | 2 +-
> 2 files changed, 64 insertions(+), 32 deletions(-)
>
> diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
> index 39334b7..edd8447 100644
> --- a/src/xen/xen_driver.c
> +++ b/src/xen/xen_driver.c
> @@ -73,12 +73,14 @@
>
> static int
> xenUnifiedNodeGetInfo(virConnectPtr conn, virNodeInfoPtr info);
> +
> static int
> -xenUnifiedDomainGetMaxVcpus(virDomainPtr dom);
> +__xenUnifiedDomainGetVcpusFlags(virDomainPtr dom, virDomainDefPtr def,
> + unsigned int flags);
>
libvirt typically uses a '*Internal' naming pattern for these types of
internal functions, e.g. xenUnifiedDomainGetVcpusFlagsInternal. Also as
we touch this code we should strive to use the libvirt pattern of
putting each parameter after the first on a new line when the list of
parameters exceeds 80 columns. Finally, since you added a line after
the xenUnifiedNodeGetInfo declaration, we should add one here too.
> static int
> -xenUnifiedDomainGetVcpus(virDomainPtr dom,
> - virVcpuInfoPtr info, int maxinfo,
> - unsigned char *cpumaps, int maplen);
> +__xenUnifiedDomainGetVcpus(virDomainPtr dom, virDomainDefPtr def,
> + virVcpuInfoPtr info, int maxinfo,
> + unsigned char *cpumaps, int maplen);
>
Same comment here.
>
>
> static bool is_privileged = false;
> @@ -173,6 +175,7 @@ xenNumaInit(virConnectPtr conn) {
> /**
> * xenDomainUsedCpus:
> * @dom: the domain
> + * @def: the domain definition
> *
> * Analyze which set of CPUs are used by the domain and
> * return a string providing the ranges.
> @@ -181,7 +184,7 @@ xenNumaInit(virConnectPtr conn) {
> * NULL if the domain uses all CPU or in case of error.
> */
> char *
> -xenDomainUsedCpus(virDomainPtr dom)
> +xenDomainUsedCpus(virDomainPtr dom, virDomainDefPtr def)
> {
> char *res = NULL;
> int ncpus;
> @@ -202,9 +205,14 @@ xenDomainUsedCpus(virDomainPtr dom)
>
> if (priv->nbNodeCpus <= 0)
> return NULL;
> - nb_vcpu = xenUnifiedDomainGetMaxVcpus(dom);
> + nb_vcpu = __xenUnifiedDomainGetVcpusFlags(dom, def,
> + (VIR_DOMAIN_VCPU_LIVE |
> + VIR_DOMAIN_VCPU_MAXIMUM));
> if (nb_vcpu <= 0)
> return NULL;
> + /* FIXME: To be consistent this should map to an internal interface, too.
> + * Currently it actually does map straight to xenDaemonNodeGetInfo().
> + */
>
I don't think this comment is necessary. Instead, just send a follow-up
patch :).
> if (xenUnifiedNodeGetInfo(dom->conn, &nodeinfo) < 0)
> return NULL;
>
> @@ -217,8 +225,8 @@ xenDomainUsedCpus(virDomainPtr dom)
> VIR_ALLOC_N(cpumap, nb_vcpu * cpumaplen) < 0)
> goto done;
>
> - if ((ncpus = xenUnifiedDomainGetVcpus(dom, cpuinfo, nb_vcpu,
> - cpumap, cpumaplen)) >= 0) {
> + if ((ncpus = __xenUnifiedDomainGetVcpus(dom, def, cpuinfo, nb_vcpu,
> + cpumap, cpumaplen)) >= 0) {
> for (n = 0; n < ncpus; n++) {
> for (m = 0; m < priv->nbNodeCpus; m++) {
> bool used;
> @@ -1410,54 +1418,57 @@ cleanup:
> }
>
> static int
> -xenUnifiedDomainGetVcpus(virDomainPtr dom,
> - virVcpuInfoPtr info, int maxinfo,
> - unsigned char *cpumaps, int maplen)
> +__xenUnifiedDomainGetVcpus(virDomainPtr dom, virDomainDefPtr def,
> + virVcpuInfoPtr info, int maxinfo,
> + unsigned char *cpumaps, int maplen)
>
Same comment here about parameters after the first being on their own
line when exceeding 80 columns.
> {
> xenUnifiedPrivatePtr priv = dom->conn->privateData;
> - virDomainDefPtr def = NULL;
> int ret = -1;
>
> - if (!(def = xenGetDomainDefForDom(dom)))
> - goto cleanup;
> -
> - if (virDomainGetVcpusEnsureACL(dom->conn, def) < 0)
> - goto cleanup;
> -
> if (dom->id < 0) {
> if (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("Cannot get VCPUs of inactive domain"));
> - goto cleanup;
> } else {
> - ret = xenDaemonDomainGetVcpus(dom->conn, def, info, maxinfo, cpumaps, maplen);
> + ret = xenDaemonDomainGetVcpus(dom->conn, def, info, maxinfo,
> + cpumaps, maplen);
> }
> } else {
> - ret = xenHypervisorGetVcpus(dom->conn, def, info, maxinfo, cpumaps, maplen);
> + ret = xenHypervisorGetVcpus(dom->conn, def, info, maxinfo, cpumaps,
> + maplen);
> }
>
> -cleanup:
> - virDomainDefFree(def);
> return ret;
> }
>
> static int
> -xenUnifiedDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags)
> +xenUnifiedDomainGetVcpus(virDomainPtr dom,
> + virVcpuInfoPtr info, int maxinfo,
> + unsigned char *cpumaps, int maplen)
>
And here.
> {
> - xenUnifiedPrivatePtr priv = dom->conn->privateData;
> virDomainDefPtr def = NULL;
> int ret = -1;
>
> - virCheckFlags(VIR_DOMAIN_VCPU_LIVE |
> - VIR_DOMAIN_VCPU_CONFIG |
> - VIR_DOMAIN_VCPU_MAXIMUM, -1);
> -
> if (!(def = xenGetDomainDefForDom(dom)))
> goto cleanup;
>
> - if (virDomainGetVcpusFlagsEnsureACL(dom->conn, def) < 0)
> + if (virDomainGetVcpusEnsureACL(dom->conn, def) < 0)
> goto cleanup;
>
> + ret = __xenUnifiedDomainGetVcpus(dom, def, info, maxinfo, cpumaps, maplen);
> +
> +cleanup:
> + virDomainDefFree(def);
> + return ret;
> +}
> +
> +static int
> +__xenUnifiedDomainGetVcpusFlags(virDomainPtr dom, virDomainDefPtr def,
> + unsigned int flags)
>
And here.
> +{
> + xenUnifiedPrivatePtr priv = dom->conn->privateData;
> + int ret = -1;
> +
> if (dom->id < 0) {
> if (priv->xendConfigVersion < XEND_CONFIG_VERSION_3_0_4)
> ret = xenXMDomainGetVcpusFlags(dom->conn, def, flags);
> @@ -1470,6 +1481,27 @@ xenUnifiedDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags)
> ret = xenDaemonDomainGetVcpusFlags(dom->conn, def, flags);
> }
>
> + return ret;
> +}
> +
> +static int
> +xenUnifiedDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags)
> +{
> + virDomainDefPtr def = NULL;
> + int ret = -1;
> +
> + virCheckFlags(VIR_DOMAIN_VCPU_LIVE |
> + VIR_DOMAIN_VCPU_CONFIG |
> + VIR_DOMAIN_VCPU_MAXIMUM, -1);
> +
> + if (!(def = xenGetDomainDefForDom(dom)))
> + goto cleanup;
> +
> + if (virDomainGetVcpusFlagsEnsureACL(dom->conn, def) < 0)
> + goto cleanup;
> +
> + ret = __xenUnifiedDomainGetVcpusFlags(dom, def, flags);
> +
> cleanup:
> virDomainDefFree(def);
> return ret;
> @@ -1501,7 +1533,7 @@ xenUnifiedDomainGetXMLDesc(virDomainPtr dom, unsigned int flags)
> } else {
> char *cpus;
> xenUnifiedLock(priv);
> - cpus = xenDomainUsedCpus(dom);
> + cpus = xenDomainUsedCpus(dom, def);
>
This should be minidef right? Otherwise def is NULL, resulting in a
segfault further down the call chain.
Regards,
Jim
> xenUnifiedUnlock(priv);
> def = xenDaemonDomainGetXMLDesc(dom->conn, minidef, cpus);
> VIR_FREE(cpus);
> diff --git a/src/xen/xen_driver.h b/src/xen/xen_driver.h
> index 3c7a8cd..a363161 100644
> --- a/src/xen/xen_driver.h
> +++ b/src/xen/xen_driver.h
> @@ -187,7 +187,7 @@ struct _xenUnifiedPrivate {
>
> typedef struct _xenUnifiedPrivate *xenUnifiedPrivatePtr;
>
> -char *xenDomainUsedCpus(virDomainPtr dom);
> +char *xenDomainUsedCpus(virDomainPtr dom, virDomainDefPtr def);
>
> virDomainXMLOptionPtr xenDomainXMLConfInit(void);
>
> -- 1.7.9.5
More information about the libvir-list
mailing list