[libvirt] [PATCH 0/4] parallels: fix different domain define parameters
Actually our checking for unsupported parameters is too strict about cpumask, vcpupin and NUMA parameters and in general wrong. First of all, we do support cpumask and consequently we don't have to prevent domain definition in case cpumask is specified. Also we should allow vcpupin parameters if they don't contradict with common cpumask. NUMA checking was also wrong because def->numa is always allocated. Here we check that its content is not specified. Maxim Nestratov (4): parallels: cpumask support parallels: don't forget to unlock domain in parallelsDomainHasManagedSaveImage parallels: prevent domain define only if NUMA is really specified parallels: prevent domain define only if vcpupin is specified
Signed-off-by: Maxim Nestratov <mnestratov@parallels.com> --- src/parallels/parallels_driver.c | 4 +--- src/parallels/parallels_sdk.c | 31 ++++++++++++++++--------------- src/parallels/parallels_utils.h | 1 - 3 files changed, 17 insertions(+), 19 deletions(-) diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 650b790..09d1cca 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -777,7 +777,6 @@ parallelsDomainGetVcpus(virDomainPtr domain, int maplen) { parallelsConnPtr privconn = domain->conn->privateData; - parallelsDomObjPtr privdomdata = NULL; virDomainObjPtr privdom = NULL; size_t i; int v, maxcpu, hostcpus; @@ -799,7 +798,6 @@ parallelsDomainGetVcpus(virDomainPtr domain, goto cleanup; } - privdomdata = privdom->privateData; if ((hostcpus = nodeGetCPUCount()) < 0) goto cleanup; @@ -820,7 +818,7 @@ parallelsDomainGetVcpus(virDomainPtr domain, int tmpmapLen = 0; memset(cpumaps, 0, maplen * maxinfo); - virBitmapToData(privdomdata->cpumask, &tmpmap, &tmpmapLen); + virBitmapToData(privdom->def->cpumask, &tmpmap, &tmpmapLen); if (tmpmapLen > maplen) tmpmapLen = maplen; diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index fec145d..5a3969e 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -412,7 +412,6 @@ prlsdkDomObjFreePrivate(void *p) return; PrlHandle_Free(pdom->sdkdom); - virBitmapFree(pdom->cpumask); VIR_FREE(pdom->uuid); VIR_FREE(pdom->home); VIR_FREE(p); @@ -1053,8 +1052,7 @@ prlsdkConvertDomainState(VIRTUAL_MACHINE_STATE domainState, static int prlsdkConvertCpuInfo(PRL_HANDLE sdkdom, - virDomainDefPtr def, - parallelsDomObjPtr pdom) + virDomainDefPtr def) { char *buf; PRL_UINT32 buflen = 0; @@ -1085,11 +1083,11 @@ prlsdkConvertCpuInfo(PRL_HANDLE sdkdom, pret = PrlVmCfg_GetCpuMask(sdkdom, buf, &buflen); if (strlen(buf) == 0) { - if (!(pdom->cpumask = virBitmapNew(hostcpus))) + if (!(def->cpumask = virBitmapNew(hostcpus))) goto cleanup; - virBitmapSetAll(pdom->cpumask); + virBitmapSetAll(def->cpumask); } else { - if (virBitmapParse(buf, 0, &pdom->cpumask, hostcpus) < 0) + if (virBitmapParse(buf, 0, &def->cpumask, hostcpus) < 0) goto cleanup; } @@ -1217,7 +1215,7 @@ prlsdkLoadDomain(parallelsConnPtr privconn, convert to Kbytes */ def->mem.cur_balloon = def->mem.max_balloon; - if (prlsdkConvertCpuInfo(sdkdom, def, pdom) < 0) + if (prlsdkConvertCpuInfo(sdkdom, def) < 0) goto error; if (prlsdkConvertCpuMode(sdkdom, def) < 0) @@ -1807,13 +1805,6 @@ prlsdkCheckUnsupportedParams(PRL_HANDLE sdkdom, virDomainDefPtr def) return -1; } - if (def->cpumask != NULL) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("changing cpu mask is not supported " - "by parallels driver")); - return -1; - } - if (def->cputune.shares || def->cputune.sharesSpecified || def->cputune.period || @@ -2842,6 +2833,7 @@ prlsdkDoApplyConfig(PRL_HANDLE sdkdom, size_t i; char uuidstr[VIR_UUID_STRING_BUFLEN + 2]; bool needBoot = true; + char *mask = NULL; if (prlsdkCheckUnsupportedParams(sdkdom, def) < 0) return -1; @@ -2869,6 +2861,13 @@ prlsdkDoApplyConfig(PRL_HANDLE sdkdom, pret = PrlVmCfg_SetCpuCount(sdkdom, def->vcpus); prlsdkCheckRetGoto(pret, error); + if (!(mask = virBitmapFormat(def->cpumask))) + goto error; + + pret = PrlVmCfg_SetCpuMask(sdkdom, mask); + prlsdkCheckRetGoto(pret, error); + VIR_FREE(mask); + if (prlsdkClearDevices(sdkdom) < 0) goto error; @@ -2912,7 +2911,9 @@ prlsdkDoApplyConfig(PRL_HANDLE sdkdom, return 0; error: - return -1; + VIR_FREE(mask); + + return -1; } int diff --git a/src/parallels/parallels_utils.h b/src/parallels/parallels_utils.h index ead5586..394548a 100644 --- a/src/parallels/parallels_utils.h +++ b/src/parallels/parallels_utils.h @@ -69,7 +69,6 @@ struct parallelsDomObj { int id; char *uuid; char *home; - virBitmapPtr cpumask; PRL_HANDLE sdkdom; }; -- 1.7.1
On Tue, Mar 10, 2015 at 23:12:19 +0300, Maxim Nestratov wrote:
Signed-off-by: Maxim Nestratov <mnestratov@parallels.com> --- src/parallels/parallels_driver.c | 4 +--- src/parallels/parallels_sdk.c | 31 ++++++++++++++++--------------- src/parallels/parallels_utils.h | 1 - 3 files changed, 17 insertions(+), 19 deletions(-)
ACK, Peter
Signed-off-by: Maxim Nestratov <mnestratov@parallels.com> --- src/parallels/parallels_driver.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 09d1cca..bbe9450 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -973,6 +973,9 @@ parallelsDomainHasManagedSaveImage(virDomainPtr domain, unsigned int flags) return -1; } + if (dom) + virObjectUnlock(dom); + return 0; } -- 1.7.1
On Tue, Mar 10, 2015 at 23:12:20 +0300, Maxim Nestratov wrote:
Signed-off-by: Maxim Nestratov <mnestratov@parallels.com> --- src/parallels/parallels_driver.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 09d1cca..bbe9450 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -973,6 +973,9 @@ parallelsDomainHasManagedSaveImage(virDomainPtr domain, unsigned int flags) return -1; }
+ if (dom)
No need to check dom here. It is guaranteed to be set at this point. As it might set off our coverity checker I'll touch up the patch.
+ virObjectUnlock(dom); + return 0; }
ACK, Peter
Signed-off-by: Maxim Nestratov <mnestratov@parallels.com> --- src/parallels/parallels_sdk.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-) diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index 5a3969e..4ec9161 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -1816,7 +1816,15 @@ prlsdkCheckUnsupportedParams(PRL_HANDLE sdkdom, virDomainDefPtr def) return -1; } - if (def->numa) { + + /* + * Though we don't support NUMA configuration at the moment + * virDomainDefPtr always contain non zero NUMA configuration + * So, just make sure this configuration does't differ from auto generated. + */ + if ((virDomainNumatuneGetMode(def->numa, -1) != + VIR_DOMAIN_NUMATUNE_MEM_STRICT) || + virDomainNumatuneHasPerNodeBinding(def->numa)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("numa parameters are not supported " "by parallels driver")); -- 1.7.1
On Tue, Mar 10, 2015 at 23:12:21 +0300, Maxim Nestratov wrote:
Signed-off-by: Maxim Nestratov <mnestratov@parallels.com> --- src/parallels/parallels_sdk.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index 5a3969e..4ec9161 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -1816,7 +1816,15 @@ prlsdkCheckUnsupportedParams(PRL_HANDLE sdkdom, virDomainDefPtr def) return -1; }
- if (def->numa) {
ACK, and sorry for breaking this code with my changes. Peter
and their settings differ from common cpumask Signed-off-by: Maxim Nestratov <mnestratov@parallels.com> --- src/parallels/parallels_sdk.c | 14 ++++++++++++-- 1 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index 4ec9161..fde908b 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -1808,14 +1808,24 @@ prlsdkCheckUnsupportedParams(PRL_HANDLE sdkdom, virDomainDefPtr def) if (def->cputune.shares || def->cputune.sharesSpecified || def->cputune.period || - def->cputune.quota || - def->cputune.nvcpupin) { + def->cputune.quota) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("cputune is not supported by parallels driver")); return -1; } + if (def->cputune.vcpupin) { + for (i = 0; i < def->vcpus; i++) { + if (!virBitmapEqual(def->cpumask, + def->cputune.vcpupin[i]->cpumask)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + "%s", _("vcpupin cpumask differs from default cpumask")); + return -1; + } + } + } + /* * Though we don't support NUMA configuration at the moment -- 1.7.1
On Tue, Mar 10, 2015 at 23:12:22 +0300, Maxim Nestratov wrote:
and their settings differ from common cpumask
Signed-off-by: Maxim Nestratov <mnestratov@parallels.com> --- src/parallels/parallels_sdk.c | 14 ++++++++++++-- 1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index 4ec9161..fde908b 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -1808,14 +1808,24 @@ prlsdkCheckUnsupportedParams(PRL_HANDLE sdkdom, virDomainDefPtr def) if (def->cputune.shares || def->cputune.sharesSpecified || def->cputune.period || - def->cputune.quota || - def->cputune.nvcpupin) { + def->cputune.quota) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("cputune is not supported by parallels driver")); return -1; }
+ if (def->cputune.vcpupin) { + for (i = 0; i < def->vcpus; i++) {
While this is still correct as the rest of the vcpupin array is filled by the default cpu pinning mask, the rest of the cpus can be skipped. ACK regardless. Peter
participants (2)
-
Maxim Nestratov -
Peter Krempa