[libvirt] [PATCH v2 0/4] parallels: fixes and cleanups

From: Maxim Nestratov <mnestratov@parallels.com> v2 change: - rebased Maxim Nestratov (3): parallels: don't forget to unlock domain if unregister fails parallels: fix home directory for VMs parallels: minor cleanup Mikhail Feoktistov (1): parallels: set cpu mode when applying xml configuration

From: Mikhail Feoktistov <mfeoktistov@parallels.com> Otherwise exporting existing domain config and defining a new one like this: virsh -c parallels:///system dumpxml instance01 > my.xml virsh -c parallels:///system define my.xml leads to an error because PCS default x64 mode turns to x32. Thus, we need to set correct cpuMode in prlsdkDoApplyConfig() explicitly. Signed-off-by: Mikhail Feoktistov <mfeoktistov@parallels.com> Signed-off-by: Maxim Nestratov <mnestratov@parallels.com> --- src/parallels/parallels_sdk.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-) diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index 4c90a18..b6026fd 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -2957,6 +2957,19 @@ prlsdkDoApplyConfig(virConnectPtr conn, prlsdkCheckRetGoto(pret, error); VIR_FREE(mask); + switch (def->os.arch) { + case VIR_ARCH_X86_64: + pret = PrlVmCfg_SetCpuMode(sdkdom, PCM_CPU_MODE_64); + break; + case VIR_ARCH_I686: + pret = PrlVmCfg_SetCpuMode(sdkdom, PCM_CPU_MODE_32); + break; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown CPU mode: %X"), def->os.arch); + goto error; + } + prlsdkCheckRetGoto(pret, error); + if (prlsdkClearDevices(sdkdom) < 0) goto error; -- 1.7.1

On 13.03.2015 16:40, Maxim Nestratov wrote:
From: Mikhail Feoktistov <mfeoktistov@parallels.com>
Otherwise exporting existing domain config and defining a new one like this: virsh -c parallels:///system dumpxml instance01 > my.xml virsh -c parallels:///system define my.xml leads to an error because PCS default x64 mode turns to x32. Thus, we need to set correct cpuMode in prlsdkDoApplyConfig() explicitly.
Signed-off-by: Mikhail Feoktistov <mfeoktistov@parallels.com> Signed-off-by: Maxim Nestratov <mnestratov@parallels.com> --- src/parallels/parallels_sdk.c | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index 4c90a18..b6026fd 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -2957,6 +2957,19 @@ prlsdkDoApplyConfig(virConnectPtr conn, prlsdkCheckRetGoto(pret, error); VIR_FREE(mask);
+ switch (def->os.arch) { + case VIR_ARCH_X86_64: + pret = PrlVmCfg_SetCpuMode(sdkdom, PCM_CPU_MODE_64); + break; + case VIR_ARCH_I686: + pret = PrlVmCfg_SetCpuMode(sdkdom, PCM_CPU_MODE_32); + break; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown CPU mode: %X"), def->os.arch);
We can use virArchToString() to report human readable cpu arch.
+ goto error; + } + prlsdkCheckRetGoto(pret, error); + if (prlsdkClearDevices(sdkdom) < 0) goto error;
ACK with this squashed in: diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index ec3372f..3a7efe3 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -2894,7 +2894,9 @@ prlsdkDoApplyConfig(PRL_HANDLE sdkdom, pret = PrlVmCfg_SetCpuMode(sdkdom, PCM_CPU_MODE_32); break; default: - virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown CPU mode: %X"), def->os.arch); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown CPU mode: %s"), + virArchToString(def->os.arch)); goto error; } prlsdkCheckRetGoto(pret, error); Michal

Signed-off-by: Maxim Nestratov <mnestratov@parallels.com> --- src/parallels/parallels_driver.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-) diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index d2907cf..aeb43ad 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -948,6 +948,7 @@ parallelsDomainUndefineFlags(virDomainPtr domain, { parallelsConnPtr privconn = domain->conn->privateData; virDomainObjPtr dom = NULL; + int ret; virCheckFlags(0, -1); @@ -957,7 +958,11 @@ parallelsDomainUndefineFlags(virDomainPtr domain, return -1; } - return prlsdkUnregisterDomain(privconn, dom); + ret = prlsdkUnregisterDomain(privconn, dom); + if (ret) + virObjectUnlock(dom); + + return ret; } static int -- 1.7.1

On 13.03.2015 16:40, Maxim Nestratov wrote:
Signed-off-by: Maxim Nestratov <mnestratov@parallels.com> --- src/parallels/parallels_driver.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index d2907cf..aeb43ad 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -948,6 +948,7 @@ parallelsDomainUndefineFlags(virDomainPtr domain, { parallelsConnPtr privconn = domain->conn->privateData; virDomainObjPtr dom = NULL; + int ret;
virCheckFlags(0, -1);
@@ -957,7 +958,11 @@ parallelsDomainUndefineFlags(virDomainPtr domain, return -1; }
- return prlsdkUnregisterDomain(privconn, dom); + ret = prlsdkUnregisterDomain(privconn, dom); + if (ret) + virObjectUnlock(dom); + + return ret; }
static int
Nice catch. ACK. Michal

Failures of parallelsStorageOpen occured because we incorrectly treated path to VM' configuration file as a directory. Now initialization of parallels VM domains home directory is fixed. Signed-off-by: Maxim Nestratov <mnestratov@parallels.com> --- src/parallels/parallels_sdk.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-) diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index b6026fd..1025da5 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -1255,6 +1255,14 @@ prlsdkLoadDomain(parallelsConnPtr privconn, pret = PrlVmCfg_GetHomePath(sdkdom, pdom->home, &buflen); prlsdkCheckRetGoto(pret, error); + /* For VMs pdom->home is actually /directory/config.pvs */ + if (!IS_CT(def)) { + /* Get rid of /config.pvs in path string */ + char *s = strrchr(pdom->home, '/'); + if (s) + *s = '\0'; + } + if (olddom) { /* assign new virDomainDef without any checks */ /* we can't use virDomainObjAssignDef, because it checks -- 1.7.1

On 13.03.2015 16:40, Maxim Nestratov wrote:
Failures of parallelsStorageOpen occured because we incorrectly treated path to VM' configuration file as a directory. Now initialization of parallels VM domains home directory is fixed.
Signed-off-by: Maxim Nestratov <mnestratov@parallels.com> --- src/parallels/parallels_sdk.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index b6026fd..1025da5 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -1255,6 +1255,14 @@ prlsdkLoadDomain(parallelsConnPtr privconn, pret = PrlVmCfg_GetHomePath(sdkdom, pdom->home, &buflen); prlsdkCheckRetGoto(pret, error);
+ /* For VMs pdom->home is actually /directory/config.pvs */ + if (!IS_CT(def)) { + /* Get rid of /config.pvs in path string */ + char *s = strrchr(pdom->home, '/'); + if (s) + *s = '\0'; + } + if (olddom) { /* assign new virDomainDef without any checks */ /* we can't use virDomainObjAssignDef, because it checks
ACK Michal

indentation is fixed, unnecessary error message removed, unnecessary job freeing removed Signed-off-by: Maxim Nestratov <mnestratov@parallels.com> --- src/parallels/parallels_sdk.c | 5 +---- 1 files changed, 1 insertions(+), 4 deletions(-) diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index 1025da5..f6350df 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -698,7 +698,7 @@ prlsdkGetNetInfo(PRL_HANDLE netAdapter, virDomainNetDefPtr net, bool isCt) goto cleanup; pret = PrlVmDevNet_GetHostInterfaceName(netAdapter, net->ifname, &buflen); - prlsdkCheckRetGoto(pret, cleanup); + prlsdkCheckRetGoto(pret, cleanup); pret = PrlVmDev_GetIndex(netAdapter, &netAdapterIndex); prlsdkCheckRetGoto(pret, cleanup); @@ -1360,7 +1360,6 @@ prlsdkLoadDomains(parallelsConnPtr privconn) error: PrlHandle_Free(result); - PrlHandle_Free(job); return -1; } @@ -1740,8 +1739,6 @@ prlsdkDomainChangeState(virDomainPtr domain, pdom = dom->privateData; pret = chstate(privconn, pdom->sdkdom); - virReportError(VIR_ERR_OPERATION_FAILED, - _("Can't change domain state: %d"), pret); if (PRL_FAILED(pret)) { virResetLastError(); -- 1.7.1

On 13.03.2015 16:40, Maxim Nestratov wrote:
indentation is fixed, unnecessary error message removed, unnecessary job freeing removed
Signed-off-by: Maxim Nestratov <mnestratov@parallels.com> --- src/parallels/parallels_sdk.c | 5 +---- 1 files changed, 1 insertions(+), 4 deletions(-)
diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index 1025da5..f6350df 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -698,7 +698,7 @@ prlsdkGetNetInfo(PRL_HANDLE netAdapter, virDomainNetDefPtr net, bool isCt) goto cleanup;
pret = PrlVmDevNet_GetHostInterfaceName(netAdapter, net->ifname, &buflen); - prlsdkCheckRetGoto(pret, cleanup); + prlsdkCheckRetGoto(pret, cleanup);
pret = PrlVmDev_GetIndex(netAdapter, &netAdapterIndex); prlsdkCheckRetGoto(pret, cleanup); @@ -1360,7 +1360,6 @@ prlsdkLoadDomains(parallelsConnPtr privconn)
error: PrlHandle_Free(result); - PrlHandle_Free(job); return -1; }
@@ -1740,8 +1739,6 @@ prlsdkDomainChangeState(virDomainPtr domain,
pdom = dom->privateData; pret = chstate(privconn, pdom->sdkdom); - virReportError(VIR_ERR_OPERATION_FAILED, - _("Can't change domain state: %d"), pret); if (PRL_FAILED(pret)) { virResetLastError();
I'm having some difficulties applying this patch. Can you rebase to the current master and resend? The problem is in the first chunk, I guess. Michal

17.03.2015 17:51, Michal Privoznik пишет:
On 13.03.2015 16:40, Maxim Nestratov wrote:
indentation is fixed, unnecessary error message removed, unnecessary job freeing removed
Signed-off-by: Maxim Nestratov <mnestratov@parallels.com> --- src/parallels/parallels_sdk.c | 5 +---- 1 files changed, 1 insertions(+), 4 deletions(-)
diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c index 1025da5..f6350df 100644 --- a/src/parallels/parallels_sdk.c +++ b/src/parallels/parallels_sdk.c @@ -698,7 +698,7 @@ prlsdkGetNetInfo(PRL_HANDLE netAdapter, virDomainNetDefPtr net, bool isCt) goto cleanup;
pret = PrlVmDevNet_GetHostInterfaceName(netAdapter, net->ifname, &buflen); - prlsdkCheckRetGoto(pret, cleanup); + prlsdkCheckRetGoto(pret, cleanup);
pret = PrlVmDev_GetIndex(netAdapter, &netAdapterIndex); prlsdkCheckRetGoto(pret, cleanup); @@ -1360,7 +1360,6 @@ prlsdkLoadDomains(parallelsConnPtr privconn)
error: PrlHandle_Free(result); - PrlHandle_Free(job); return -1; }
@@ -1740,8 +1739,6 @@ prlsdkDomainChangeState(virDomainPtr domain,
pdom = dom->privateData; pret = chstate(privconn, pdom->sdkdom); - virReportError(VIR_ERR_OPERATION_FAILED, - _("Can't change domain state: %d"), pret); if (PRL_FAILED(pret)) { virResetLastError();
I'm having some difficulties applying this patch. Can you rebase to the current master and resend? The problem is in the first chunk, I guess.
Michal Sure. In a minute.

On 13.03.2015 16:40, Maxim Nestratov wrote:
From: Maxim Nestratov <mnestratov@parallels.com>
v2 change: - rebased
Maxim Nestratov (3): parallels: don't forget to unlock domain if unregister fails parallels: fix home directory for VMs parallels: minor cleanup
Mikhail Feoktistov (1): parallels: set cpu mode when applying xml configuration
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
I've ACKed and pushed the first three patches. Michal
participants (2)
-
Maxim Nestratov
-
Michal Privoznik