9:38 a.m.
Patches 1 and 2 make preparation for patch 4
Patch 3 fixes notification subscription
To get domain info we should receive and handle notification from hypervisor
Patch 4 fixes race condition when adding domain to domains list
Race condition:
User calls defineXML to create new instance.
The main thread from vzDomainDefineXMLFlags() creates new instance by prlsdkCreateVm.
Then this thread calls prlsdkAddDomain to add new domain to domains list.
The second thread receives notification from hypervisor that new VM was created.
It calls prlsdkHandleVmAddedEvent() and also tries to add new domain to domains list.
These two threads call virDomainObjListFindByUUID() from prlsdkAddDomain() and don't
find new domain.
So they add two domains with the same uuid to domains list.
Mikhail Feoktistov (4):
vz: make output arguments in prlsdkGetDomainIds as optional
vz: remove unused struct field
vz: fix notification subscription
vz: fix race condition when adding domain to domains list
src/vz/vz_driver.c | 13 ++-
src/vz/vz_sdk.c | 293 +++++++++++++++++++++++++++--------------------------
src/vz/vz_sdk.h | 9 +-
src/vz/vz_utils.h | 1 -
4 files changed, 165 insertions(+), 151 deletions(-)
--
1.8.3.1
9:38 a.m.
New subject: [libvirt] [PATCH 1/4] vz: make output arguments in prlsdkGetDomainIds as optional
prlsdkGetDomainIds() returns name and uuid for specified instance.
Now output arguments can be NULL.
It allows to get only necessary info(name or uuid).
---
src/vz/vz_sdk.c | 34 +++++++++++++++++++---------------
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 7973d6a..8181149 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -346,25 +346,29 @@ prlsdkGetDomainIds(PRL_HANDLE sdkdom,
PRL_UINT32 len;
PRL_RESULT pret;
- len = 0;
- /* get name length */
- pret = PrlVmCfg_GetName(sdkdom, NULL, &len);
- prlsdkCheckRetGoto(pret, error);
+ if (name) {
+ len = 0;
+ /* get name length */
+ pret = PrlVmCfg_GetName(sdkdom, NULL, &len);
+ prlsdkCheckRetGoto(pret, error);
- if (VIR_ALLOC_N(*name, len) < 0)
- goto error;
+ if (VIR_ALLOC_N(*name, len) < 0)
+ goto error;
- PrlVmCfg_GetName(sdkdom, *name, &len);
- prlsdkCheckRetGoto(pret, error);
+ PrlVmCfg_GetName(sdkdom, *name, &len);
+ prlsdkCheckRetGoto(pret, error);
+ }
- len = sizeof(uuidstr);
- PrlVmCfg_GetUuid(sdkdom, uuidstr, &len);
- prlsdkCheckRetGoto(pret, error);
+ if (uuid) {
+ len = sizeof(uuidstr);
+ PrlVmCfg_GetUuid(sdkdom, uuidstr, &len);
+ prlsdkCheckRetGoto(pret, error);
- if (prlsdkUUIDParse(uuidstr, uuid) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Domain UUID is malformed or empty"));
- goto error;
+ if (prlsdkUUIDParse(uuidstr, uuid) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Domain UUID is malformed or empty"));
+ goto error;
+ }
}
return 0;
--
1.8.3.1
12:27 a.m.
New subject: [libvirt] [PATCH 1/4] vz: make output arguments in prlsdkGetDomainIds as optional
On 28.01.2016 18:38, Mikhail Feoktistov wrote:
prlsdkGetDomainIds() returns name and uuid for specified instance.
Now output arguments can be NULL.
It allows to get only necessary info(name or uuid).
---
src/vz/vz_sdk.c | 34 +++++++++++++++++++---------------
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 7973d6a..8181149 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -346,25 +346,29 @@ prlsdkGetDomainIds(PRL_HANDLE sdkdom,
PRL_UINT32 len;
PRL_RESULT pret;
- len = 0;
- /* get name length */
- pret = PrlVmCfg_GetName(sdkdom, NULL, &len);
- prlsdkCheckRetGoto(pret, error);
+ if (name) {
+ len = 0;
+ /* get name length */
+ pret = PrlVmCfg_GetName(sdkdom, NULL, &len);
+ prlsdkCheckRetGoto(pret, error);
- if (VIR_ALLOC_N(*name, len) < 0)
- goto error;
+ if (VIR_ALLOC_N(*name, len) < 0)
+ goto error;
- PrlVmCfg_GetName(sdkdom, *name, &len);
- prlsdkCheckRetGoto(pret, error);
+ PrlVmCfg_GetName(sdkdom, *name, &len);
+ prlsdkCheckRetGoto(pret, error);
+ }
- len = sizeof(uuidstr);
- PrlVmCfg_GetUuid(sdkdom, uuidstr, &len);
- prlsdkCheckRetGoto(pret, error);
+ if (uuid) {
+ len = sizeof(uuidstr);
+ PrlVmCfg_GetUuid(sdkdom, uuidstr, &len);
+ prlsdkCheckRetGoto(pret, error);
- if (prlsdkUUIDParse(uuidstr, uuid) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Domain UUID is malformed or empty"));
- goto error;
+ if (prlsdkUUIDParse(uuidstr, uuid) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Domain UUID is malformed or empty"));
+ goto error;
+ }
}
return 0;
ACK, but I would add *name = NULL as another patch or we depend on
caller setting it for us if we follow error path.
6:29 a.m.
New subject: [libvirt] [PATCH 1/4] vz: make output arguments in prlsdkGetDomainIds as optional
> prlsdkGetDomainIds() returns name and uuid for specified
instance.
> Now output arguments can be NULL.
> It allows to get only necessary info(name or uuid).
> ---
> src/vz/vz_sdk.c | 34 +++++++++++++++++++---------------
> 1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
> index 7973d6a..8181149 100644
> --- a/src/vz/vz_sdk.c
> +++ b/src/vz/vz_sdk.c
> @@ -346,25 +346,29 @@ prlsdkGetDomainIds(PRL_HANDLE sdkdom,
> PRL_UINT32 len;
> PRL_RESULT pret;
>
> - len = 0;
> - /* get name length */
> - pret = PrlVmCfg_GetName(sdkdom, NULL, &len);
> - prlsdkCheckRetGoto(pret, error);
> + if (name) {
> + len = 0;
> + /* get name length */
> + pret = PrlVmCfg_GetName(sdkdom, NULL, &len);
> + prlsdkCheckRetGoto(pret, error);
>
> - if (VIR_ALLOC_N(*name, len) < 0)
> - goto error;
> + if (VIR_ALLOC_N(*name, len) < 0)
> + goto error;
>
> - PrlVmCfg_GetName(sdkdom, *name, &len);
> - prlsdkCheckRetGoto(pret, error);
> + PrlVmCfg_GetName(sdkdom, *name, &len);
> + prlsdkCheckRetGoto(pret, error);
> + }
>
> - len = sizeof(uuidstr);
> - PrlVmCfg_GetUuid(sdkdom, uuidstr, &len);
> - prlsdkCheckRetGoto(pret, error);
> + if (uuid) {
> + len = sizeof(uuidstr);
> + PrlVmCfg_GetUuid(sdkdom, uuidstr, &len);
> + prlsdkCheckRetGoto(pret, error);
>
> - if (prlsdkUUIDParse(uuidstr, uuid) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Domain UUID is malformed or empty"));
> - goto error;
> + if (prlsdkUUIDParse(uuidstr, uuid) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Domain UUID is malformed or empty"));
> + goto error;
> + }
> }
>
> return 0;
>
ACK, but I would add *name = NULL as another patch or we depend on
caller setting it for us if we follow error path.
Done. Thanks!
2:25 a.m.
New subject: [libvirt] [PATCH 1/4] vz: make output arguments in prlsdkGetDomainIds as optional
28.01.2016 18:38, Mikhail Feoktistov пишет:
prlsdkGetDomainIds() returns name and uuid for specified instance.
Now output arguments can be NULL.
It allows to get only necessary info(name or uuid).
---
src/vz/vz_sdk.c | 34 +++++++++++++++++++---------------
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 7973d6a..8181149 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -346,25 +346,29 @@ prlsdkGetDomainIds(PRL_HANDLE sdkdom,
PRL_UINT32 len;
PRL_RESULT pret;
- len = 0;
- /* get name length */
- pret = PrlVmCfg_GetName(sdkdom, NULL, &len);
- prlsdkCheckRetGoto(pret, error);
+ if (name) {
+ len = 0;
+ /* get name length */
+ pret = PrlVmCfg_GetName(sdkdom, NULL, &len);
+ prlsdkCheckRetGoto(pret, error);
- if (VIR_ALLOC_N(*name, len) < 0)
- goto error;
+ if (VIR_ALLOC_N(*name, len) < 0)
+ goto error;
- PrlVmCfg_GetName(sdkdom, *name, &len);
- prlsdkCheckRetGoto(pret, error);
+ PrlVmCfg_GetName(sdkdom, *name, &len);
I know it's not your mistake, but it would be better not to ignore what
PrlVmCfg_GetName returns.
Otherwise next statement has no sence.
+ prlsdkCheckRetGoto(pret, error);
+ }
- len = sizeof(uuidstr);
- PrlVmCfg_GetUuid(sdkdom, uuidstr, &len);
- prlsdkCheckRetGoto(pret, error);
+ if (uuid) {
+ len = sizeof(uuidstr);
+ PrlVmCfg_GetUuid(sdkdom, uuidstr, &len);
The same is here.
+ prlsdkCheckRetGoto(pret, error);
- if (prlsdkUUIDParse(uuidstr, uuid) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Domain UUID is malformed or empty"));
- goto error;
+ if (prlsdkUUIDParse(uuidstr, uuid) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Domain UUID is malformed or empty"));
+ goto error;
+ }
}
return 0;
4:30 a.m.
New subject: [libvirt] [PATCH 1/4] vz: make output arguments in prlsdkGetDomainIds as optional
29.01.2016 11:25, Maxim Nestratov пишет:
28.01.2016 18:38, Mikhail Feoktistov пишет:
> prlsdkGetDomainIds() returns name and uuid for specified instance.
> Now output arguments can be NULL.
> It allows to get only necessary info(name or uuid).
> ---
> src/vz/vz_sdk.c | 34 +++++++++++++++++++---------------
> 1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
> index 7973d6a..8181149 100644
> --- a/src/vz/vz_sdk.c
> +++ b/src/vz/vz_sdk.c
> @@ -346,25 +346,29 @@ prlsdkGetDomainIds(PRL_HANDLE sdkdom,
> PRL_UINT32 len;
> PRL_RESULT pret;
> - len = 0;
> - /* get name length */
> - pret = PrlVmCfg_GetName(sdkdom, NULL, &len);
> - prlsdkCheckRetGoto(pret, error);
> + if (name) {
> + len = 0;
> + /* get name length */
> + pret = PrlVmCfg_GetName(sdkdom, NULL, &len);
> + prlsdkCheckRetGoto(pret, error);
> - if (VIR_ALLOC_N(*name, len) < 0)
> - goto error;
> + if (VIR_ALLOC_N(*name, len) < 0)
> + goto error;
> - PrlVmCfg_GetName(sdkdom, *name, &len);
> - prlsdkCheckRetGoto(pret, error);
> + PrlVmCfg_GetName(sdkdom, *name, &len);
I know it's not your mistake, but it would be better not to ignore
what PrlVmCfg_GetName returns.
Otherwise next statement has no sence.
Yes, of cource. Thanks!
> + prlsdkCheckRetGoto(pret, error);
> + }
> - len = sizeof(uuidstr);
> - PrlVmCfg_GetUuid(sdkdom, uuidstr, &len);
> - prlsdkCheckRetGoto(pret, error);
> + if (uuid) {
> + len = sizeof(uuidstr);
> + PrlVmCfg_GetUuid(sdkdom, uuidstr, &len);
The same is here.
> + prlsdkCheckRetGoto(pret, error);
> - if (prlsdkUUIDParse(uuidstr, uuid) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Domain UUID is malformed or empty"));
> - goto error;
> + if (prlsdkUUIDParse(uuidstr, uuid) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Domain UUID is malformed or empty"));
> + goto error;
> + }
> }
> return 0;
9:38 a.m.
New subject: [libvirt] [PATCH 2/4] vz: remove unused struct field
In commit 7039bb3c we have removed code that saves uuid to vzDomObj.uuid
So this field is no longer needed.
---
src/vz/vz_sdk.c | 5 -----
src/vz/vz_utils.h | 1 -
2 files changed, 6 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 8181149..40ab2d9 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -417,7 +417,6 @@ prlsdkDomObjFreePrivate(void *p)
PrlHandle_Free(pdom->sdkdom);
PrlHandle_Free(pdom->cache.stats);
virCondDestroy(&pdom->cache.cond);
- VIR_FREE(pdom->uuid);
VIR_FREE(pdom->home);
VIR_FREE(p);
};
@@ -1282,10 +1281,6 @@ prlsdkLoadDomain(vzConnPtr privconn,
def->id = -1;
- /* we will remove this field in the near future, so let's set it
- * to NULL temporarily */
- pdom->uuid = NULL;
-
pdom->cache.stats = PRL_INVALID_HANDLE;
pdom->cache.count = -1;
if (virCondInit(&pdom->cache.cond) < 0) {
diff --git a/src/vz/vz_utils.h b/src/vz/vz_utils.h
index 84cf08f..0fbbfc7 100644
--- a/src/vz/vz_utils.h
+++ b/src/vz/vz_utils.h
@@ -75,7 +75,6 @@ typedef struct _vzCountersCache vzCountersCache;
struct vzDomObj {
int id;
- char *uuid;
char *home;
PRL_HANDLE sdkdom;
vzCountersCache cache;
--
1.8.3.1
12:30 a.m.
New subject: [libvirt] [PATCH 2/4] vz: remove unused struct field
On 28.01.2016 18:38, Mikhail Feoktistov wrote:
In commit 7039bb3c we have removed code that saves uuid to
vzDomObj.uuid
So this field is no longer needed.
---
src/vz/vz_sdk.c | 5 -----
src/vz/vz_utils.h | 1 -
2 files changed, 6 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 8181149..40ab2d9 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -417,7 +417,6 @@ prlsdkDomObjFreePrivate(void *p)
PrlHandle_Free(pdom->sdkdom);
PrlHandle_Free(pdom->cache.stats);
virCondDestroy(&pdom->cache.cond);
- VIR_FREE(pdom->uuid);
VIR_FREE(pdom->home);
VIR_FREE(p);
};
@@ -1282,10 +1281,6 @@ prlsdkLoadDomain(vzConnPtr privconn,
def->id = -1;
- /* we will remove this field in the near future, so let's set it
- * to NULL temporarily */
- pdom->uuid = NULL;
-
pdom->cache.stats = PRL_INVALID_HANDLE;
pdom->cache.count = -1;
if (virCondInit(&pdom->cache.cond) < 0) {
diff --git a/src/vz/vz_utils.h b/src/vz/vz_utils.h
index 84cf08f..0fbbfc7 100644
--- a/src/vz/vz_utils.h
+++ b/src/vz/vz_utils.h
@@ -75,7 +75,6 @@ typedef struct _vzCountersCache vzCountersCache;
struct vzDomObj {
int id;
- char *uuid;
char *home;
PRL_HANDLE sdkdom;
vzCountersCache cache;
ACK
9:38 a.m.
New subject: [libvirt] [PATCH 3/4] vz: fix notification subscription
Bug cause:
Update the domain that is subscribed to hypervisor notification.
LoadDomain() rewrites notifications fields in vzDomObj structure and makes domain as
"unsubscribed".
Fix:
Initialize notification fields in vzDomObj only if we create a new domain.
And do not reinitialize these fields if we update domain (by calling LoadDomain with
olddom argument)
---
src/vz/vz_sdk.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 40ab2d9..6fb2a97 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -1270,6 +1270,12 @@ prlsdkLoadDomain(vzConnPtr privconn,
if (!olddom) {
if (VIR_ALLOC(pdom) < 0)
goto error;
+ pdom->cache.stats = PRL_INVALID_HANDLE;
+ pdom->cache.count = -1;
+ if (virCondInit(&pdom->cache.cond) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot
initialize condition"));
+ goto error;
+ }
} else {
pdom = olddom->privateData;
}
@@ -1281,13 +1287,6 @@ prlsdkLoadDomain(vzConnPtr privconn,
def->id = -1;
- pdom->cache.stats = PRL_INVALID_HANDLE;
- pdom->cache.count = -1;
- if (virCondInit(&pdom->cache.cond) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize
condition"));
- goto error;
- }
-
if (prlsdkGetDomainIds(sdkdom, &def->name, def->uuid) < 0)
goto error;
--
1.8.3.1
12:33 a.m.
New subject: [libvirt] [PATCH 3/4] vz: fix notification subscription
On 28.01.2016 18:38, Mikhail Feoktistov wrote:
Bug cause:
Update the domain that is subscribed to hypervisor notification.
LoadDomain() rewrites notifications fields in vzDomObj structure and makes domain as
"unsubscribed".
Fix:
Initialize notification fields in vzDomObj only if we create a new domain.
And do not reinitialize these fields if we update domain (by calling LoadDomain with
olddom argument)
---
src/vz/vz_sdk.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 40ab2d9..6fb2a97 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -1270,6 +1270,12 @@ prlsdkLoadDomain(vzConnPtr privconn,
if (!olddom) {
if (VIR_ALLOC(pdom) < 0)
goto error;
+ pdom->cache.stats = PRL_INVALID_HANDLE;
+ pdom->cache.count = -1;
+ if (virCondInit(&pdom->cache.cond) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot
initialize condition"));
+ goto error;
+ }
} else {
pdom = olddom->privateData;
}
@@ -1281,13 +1287,6 @@ prlsdkLoadDomain(vzConnPtr privconn,
def->id = -1;
- pdom->cache.stats = PRL_INVALID_HANDLE;
- pdom->cache.count = -1;
- if (virCondInit(&pdom->cache.cond) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize
condition"));
- goto error;
- }
-
if (prlsdkGetDomainIds(sdkdom, &def->name, def->uuid) < 0)
goto error;
ACK
2:58 a.m.
New subject: [libvirt] [PATCH 3/4] vz: fix notification subscription
28.01.2016 18:38, Mikhail Feoktistov пишет:
Bug cause:
Update the domain that is subscribed to hypervisor notification.
LoadDomain() rewrites notifications fields in vzDomObj structure and makes domain as
"unsubscribed".
Fix:
Initialize notification fields in vzDomObj only if we create a new domain.
And do not reinitialize these fields if we update domain (by calling LoadDomain with
olddom argument)
---
src/vz/vz_sdk.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 40ab2d9..6fb2a97 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -1270,6 +1270,12 @@ prlsdkLoadDomain(vzConnPtr privconn,
if (!olddom) {
if (VIR_ALLOC(pdom) < 0)
goto error;
+ pdom->cache.stats = PRL_INVALID_HANDLE;
+ pdom->cache.count = -1;
+ if (virCondInit(&pdom->cache.cond) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot
initialize condition"));
+ goto error;
+ }
} else {
pdom = olddom->privateData;
}
@@ -1281,13 +1287,6 @@ prlsdkLoadDomain(vzConnPtr privconn,
def->id = -1;
- pdom->cache.stats = PRL_INVALID_HANDLE;
- pdom->cache.count = -1;
- if (virCondInit(&pdom->cache.cond) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize
condition"));
- goto error;
- }
-
if (prlsdkGetDomainIds(sdkdom, &def->name, def->uuid) < 0)
goto error;
Actually I'm not sure this patch is necessary at all as soon as you
rewrite this function in the next patch.
The problem you are fixing could be just mentioned there.
9:38 a.m.
New subject: [libvirt] [PATCH 4/4] vz: fix race condition when adding domain to domains list
Race condition:
User calls defineXML to create new instance.
The main thread from vzDomainDefineXMLFlags() creates new instance by prlsdkCreateVm.
Then this thread calls prlsdkAddDomain to add new domain to domains list.
The second thread receives notification from hypervisor that new VM was created.
It calls prlsdkHandleVmAddedEvent() and also tries to add new domain to domains list.
These two threads call virDomainObjListFindByUUID() from prlsdkAddDomain() and don't
find new domain.
So they add two domains with the same uuid to domains list.
This fix splits logic of prlsdkAddDomain() into two functions.
1. prlsdkNewDomain() creates new empty domain in domains list with the specific uuid.
2. prlsdkLoadDomain() add data from VM to domain object.
New algorithm for creating an instance:
In vzDomainDefineXMLFlags() we add new domain to domain list by calling prlsdkNewDomain()
and only after that we call CreateVm() to create VM.
It means that we "reserve" domain object with the specific uuid.
After creation of new VM we add info from this VM
to reserved domain object by calling prlsdkLoadDomain().
Before this patch prlsdkLoadDomain() worked in 2 different cases:
1. It creates and initializes new domain. Then updates it from sdk handle.
2. It updates existed domain from sdk handle.
In this patch we remove code which creates new domain from LoadDomain()
and move it to prlsdkNewDomain().
Now prlsdkLoadDomain() only updates domain from skd handle.
In notification handler prlsdkHandleVmAddedEvent() we check
the existence of a domain and if it doesn't exist we add new domain by calling
prlsdkNewDomain() and load info from sdk handle via prlsdkLoadDomain().
---
src/vz/vz_driver.c | 13 ++-
src/vz/vz_sdk.c | 253 +++++++++++++++++++++++++++--------------------------
src/vz/vz_sdk.h | 9 +-
3 files changed, 146 insertions(+), 129 deletions(-)
diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 91a48b6..521efd4 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -685,6 +685,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned
int flags)
virDomainPtr retdom = NULL;
virDomainDefPtr def;
virDomainObjPtr olddom = NULL;
+ virDomainObjPtr newdom = NULL;
unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
@@ -700,6 +701,9 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned
int flags)
olddom = virDomainObjListFindByUUID(privconn->domains, def->uuid);
if (olddom == NULL) {
virResetLastError();
+ newdom = prlsdkNewDomain(privconn, def->name, def->uuid);
+ if (!newdom)
+ goto cleanup;
if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
if (prlsdkCreateVm(conn, def))
goto cleanup;
@@ -713,8 +717,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned
int flags)
goto cleanup;
}
- olddom = prlsdkAddDomain(privconn, def->uuid);
- if (!olddom)
+ if (prlsdkLoadDomain(privconn, newdom))
goto cleanup;
} else {
int state, reason;
@@ -755,6 +758,12 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned
int flags)
cleanup:
if (olddom)
virObjectUnlock(olddom);
+ if (newdom) {
+ if (!retdom)
+ virDomainObjListRemove(privconn->domains, newdom);
+ else
+ virObjectUnlock(newdom);
+ }
virDomainDefFree(def);
vzDriverUnlock(privconn);
return retdom;
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 6fb2a97..9d2bdab 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -1236,58 +1236,35 @@ prlsdkConvertCpuMode(PRL_HANDLE sdkdom, virDomainDefPtr def)
return -1;
}
-/*
- * This function retrieves information about domain.
- * If the domains is already in the domains list
- * privconn->domains, then locked 'olddom' must be
- * provided. If the domains must be added to the list,
- * olddom must be NULL.
- *
- * The function return a pointer to a locked virDomainObj.
- */
-static virDomainObjPtr
-prlsdkLoadDomain(vzConnPtr privconn,
- PRL_HANDLE sdkdom,
- virDomainObjPtr olddom)
+int
+prlsdkLoadDomain(vzConnPtr privconn, virDomainObjPtr dom)
{
- virDomainObjPtr dom = NULL;
virDomainDefPtr def = NULL;
- vzDomObjPtr pdom = NULL;
VIRTUAL_MACHINE_STATE domainState;
+ char *home = NULL;
PRL_UINT32 buflen = 0;
PRL_RESULT pret;
PRL_UINT32 ram;
PRL_UINT32 envId;
PRL_VM_AUTOSTART_OPTION autostart;
+ PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
+ vzDomObjPtr pdom = dom->privateData;
virCheckNonNullArgGoto(privconn, error);
- virCheckNonNullArgGoto(sdkdom, error);
+ virCheckNonNullArgGoto(dom, error);
+
+ sdkdom = prlsdkSdkDomainLookupByUUID(privconn, dom->def->uuid);
+ if (sdkdom == PRL_INVALID_HANDLE)
+ return -1;
if (!(def = virDomainDefNew()))
goto error;
- if (!olddom) {
- if (VIR_ALLOC(pdom) < 0)
- goto error;
- pdom->cache.stats = PRL_INVALID_HANDLE;
- pdom->cache.count = -1;
- if (virCondInit(&pdom->cache.cond) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot
initialize condition"));
- goto error;
- }
- } else {
- pdom = olddom->privateData;
- }
+ def->virtType = dom->def->virtType;
+ def->id = dom->def->id;
- if (STREQ(privconn->drivername, "vz"))
- def->virtType = VIR_DOMAIN_VIRT_VZ;
- else
- def->virtType = VIR_DOMAIN_VIRT_PARALLELS;
-
- def->id = -1;
-
- if (prlsdkGetDomainIds(sdkdom, &def->name, def->uuid) < 0)
+ if (prlsdkGetDomainIds(sdkdom, &def->name, def->uuid))
goto error;
def->onReboot = VIR_DOMAIN_LIFECYCLE_RESTART;
@@ -1318,29 +1295,33 @@ prlsdkLoadDomain(vzConnPtr privconn,
pret = PrlVmCfg_GetEnvId(sdkdom, &envId);
prlsdkCheckRetGoto(pret, error);
- pdom->id = envId;
buflen = 0;
pret = PrlVmCfg_GetHomePath(sdkdom, NULL, &buflen);
prlsdkCheckRetGoto(pret, error);
- VIR_FREE(pdom->home);
- if (VIR_ALLOC_N(pdom->home, buflen) < 0)
+ if (VIR_ALLOC_N(home, buflen) < 0)
goto error;
- pret = PrlVmCfg_GetHomePath(sdkdom, pdom->home, &buflen);
+ pret = PrlVmCfg_GetHomePath(sdkdom, home, &buflen);
prlsdkCheckRetGoto(pret, error);
- /* For VMs pdom->home is actually /directory/config.pvs */
+ /* For VMs home is actually /directory/config.pvs */
if (!IS_CT(def)) {
/* Get rid of /config.pvs in path string */
- char *s = strrchr(pdom->home, '/');
+ char *s = strrchr(home, '/');
if (s)
*s = '\0';
}
pret = PrlVmCfg_GetAutoStart(sdkdom, &autostart);
prlsdkCheckRetGoto(pret, error);
+ if (autostart != PAO_VM_START_ON_LOAD &&
+ autostart != PAO_VM_START_MANUAL) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unknown autostart mode: %X"), autostart);
+ goto error;
+ }
if (prlsdkGetDomainState(sdkdom, &domainState) < 0)
goto error;
@@ -1363,38 +1344,6 @@ prlsdkLoadDomain(vzConnPtr privconn,
goto error;
}
- if (olddom) {
- /* assign new virDomainDef without any checks */
- /* we can't use virDomainObjAssignDef, because it checks
- * for state and domain name */
- dom = olddom;
- virDomainDefFree(dom->def);
- dom->def = def;
- } else {
- if (!(dom = virDomainObjListAdd(privconn->domains, def,
- privconn->xmlopt,
- 0, NULL)))
- goto error;
- }
- /* dom is locked here */
-
- dom->privateData = pdom;
- dom->privateDataFreeFunc = prlsdkDomObjFreePrivate;
- dom->persistent = 1;
-
- switch (autostart) {
- case PAO_VM_START_ON_LOAD:
- dom->autostart = 1;
- break;
- case PAO_VM_START_MANUAL:
- dom->autostart = 0;
- break;
- default:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unknown autostart mode: %X"), autostart);
- goto error;
- }
-
if (prlsdkConvertDomainState(domainState, envId, dom) < 0)
goto error;
@@ -1404,24 +1353,27 @@ prlsdkLoadDomain(vzConnPtr privconn,
pdom->sdkdom = sdkdom;
}
- return dom;
- error:
- if (dom && !olddom) {
- /* Domain isn't persistent means that we haven't yet set
- * prlsdkDomObjFreePrivate and should call it manually
- */
- if (!dom->persistent)
- prlsdkDomObjFreePrivate(pdom);
+ /* assign new virDomainDef without any checks
+ * we can't use virDomainObjAssignDef, because it checks
+ * for state and domain name */
+ virDomainDefFree(dom->def);
+ dom->def = def;
+ pdom->id = envId;
+ VIR_FREE(pdom->home);
+ pdom->home = home;
- virDomainObjListRemove(privconn->domains, dom);
- }
- /* Delete newly allocated def only if we haven't assigned it to domain
- * Otherwise we will end up with domain having invalid def within it
- */
- if (!dom)
- virDomainDefFree(def);
+ if (autostart == PAO_VM_START_ON_LOAD)
+ dom->autostart = 1;
+ else
+ dom->autostart = 0;
- return NULL;
+ PrlHandle_Free(sdkdom);
+ return 0;
+ error:
+ PrlHandle_Free(sdkdom);
+ VIR_FREE(home);
+ virDomainDefFree(def);
+ return -1;
}
int
@@ -1429,11 +1381,13 @@ prlsdkLoadDomains(vzConnPtr privconn)
{
PRL_HANDLE job = PRL_INVALID_HANDLE;
PRL_HANDLE result;
- PRL_HANDLE sdkdom;
+ PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
PRL_UINT32 paramsCount;
PRL_RESULT pret;
size_t i = 0;
virDomainObjPtr dom;
+ unsigned char uuid[VIR_UUID_BUFLEN];
+ char *name = NULL;
job = PrlSrv_GetVmListEx(privconn->server, PVTF_VM | PVTF_CT);
@@ -1447,61 +1401,89 @@ prlsdkLoadDomains(vzConnPtr privconn)
pret = PrlResult_GetParamByIndex(result, i, &sdkdom);
if (PRL_FAILED(pret)) {
logPrlError(pret);
- PrlHandle_Free(sdkdom);
goto error;
}
- dom = prlsdkLoadDomain(privconn, sdkdom, NULL);
- PrlHandle_Free(sdkdom);
+ if (prlsdkGetDomainIds(sdkdom, &name, uuid))
+ goto error;
- if (!dom)
+ if (!(dom = prlsdkNewDomain(privconn, name, uuid)))
goto error;
- else
- virObjectUnlock(dom);
+
+ if (prlsdkLoadDomain(privconn, dom)) {
+ virDomainObjListRemove(privconn->domains, dom);
+ goto error;
+ }
+
+ VIR_FREE(name);
+ PrlHandle_Free(sdkdom);
+ sdkdom = PRL_INVALID_HANDLE;
+ virObjectUnlock(dom);
}
PrlHandle_Free(result);
return 0;
error:
+ VIR_FREE(name);
+ PrlHandle_Free(sdkdom);
PrlHandle_Free(result);
return -1;
}
-virDomainObjPtr
-prlsdkAddDomain(vzConnPtr privconn, const unsigned char *uuid)
-{
- PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
- virDomainObjPtr dom;
-
- dom = virDomainObjListFindByUUID(privconn->domains, uuid);
- if (dom) {
- /* domain is already in the list */
- return dom;
- }
-
- sdkdom = prlsdkSdkDomainLookupByUUID(privconn, uuid);
- if (sdkdom == PRL_INVALID_HANDLE)
- return NULL;
-
- dom = prlsdkLoadDomain(privconn, sdkdom, NULL);
- PrlHandle_Free(sdkdom);
- return dom;
-}
-
int
prlsdkUpdateDomain(vzConnPtr privconn, virDomainObjPtr dom)
{
PRL_HANDLE job;
- virDomainObjPtr retdom = NULL;
vzDomObjPtr pdom = dom->privateData;
job = PrlVm_RefreshConfig(pdom->sdkdom);
if (waitJob(job))
return -1;
- retdom = prlsdkLoadDomain(privconn, pdom->sdkdom, dom);
- return retdom ? 0 : -1;
+ return prlsdkLoadDomain(privconn, dom);
+}
+
+
+virDomainObjPtr
+prlsdkNewDomain(vzConnPtr privconn, char *name, const unsigned char *uuid)
+{
+ virDomainDefPtr def = NULL;
+ virDomainObjPtr dom = NULL;
+ vzDomObjPtr pdom = NULL;
+
+ if (!(def = virDomainDefNewFull(name, uuid, -1)))
+ goto error;
+
+ if (VIR_ALLOC(pdom) < 0)
+ goto error;
+
+ pdom->cache.stats = PRL_INVALID_HANDLE;
+ pdom->cache.count = -1;
+ if (virCondInit(&pdom->cache.cond) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize
condition"));
+ goto error;
+ }
+
+ if (STREQ(privconn->drivername, "vz"))
+ def->virtType = VIR_DOMAIN_VIRT_VZ;
+ else
+ def->virtType = VIR_DOMAIN_VIRT_PARALLELS;
+
+ if (!(dom = virDomainObjListAdd(privconn->domains, def,
+ privconn->xmlopt,
+ 0, NULL)))
+ goto error;
+
+ dom->privateData = pdom;
+ dom->privateDataFreeFunc = prlsdkDomObjFreePrivate;
+ dom->persistent = 1;
+ return dom;
+
+ error:
+ virDomainDefFree(def);
+ VIR_FREE(pdom);
+ return NULL;
}
static int prlsdkSendEvent(vzConnPtr privconn,
@@ -1618,15 +1600,36 @@ prlsdkHandleVmAddedEvent(vzConnPtr privconn,
unsigned char *uuid)
{
virDomainObjPtr dom = NULL;
+ PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
+ char *name = NULL;
- dom = prlsdkAddDomain(privconn, uuid);
- if (dom == NULL)
- return;
+ dom = virDomainObjListFindByUUID(privconn->domains, uuid);
+ if (!dom) {
+ sdkdom = prlsdkSdkDomainLookupByUUID(privconn, uuid);
+ if (sdkdom == PRL_INVALID_HANDLE)
+ goto cleanup;
+
+ if (prlsdkGetDomainIds(sdkdom, &name, NULL))
+ goto cleanup;
+
+ if (!(dom = prlsdkNewDomain(privconn, name, uuid)))
+ goto cleanup;
+
+ if (prlsdkLoadDomain(privconn, dom)) {
+ virDomainObjListRemove(privconn->domains, dom);
+ dom = NULL;
+ goto cleanup;
+ }
+ }
prlsdkSendEvent(privconn, dom, VIR_DOMAIN_EVENT_DEFINED,
VIR_DOMAIN_EVENT_DEFINED_ADDED);
- virObjectUnlock(dom);
+ cleanup:
+ if (dom)
+ virObjectUnlock(dom);
+ VIR_FREE(name);
+ PrlHandle_Free(sdkdom);
return;
}
diff --git a/src/vz/vz_sdk.h b/src/vz/vz_sdk.h
index ff6be07..f2d3e35 100644
--- a/src/vz/vz_sdk.h
+++ b/src/vz/vz_sdk.h
@@ -30,9 +30,14 @@ int prlsdkConnect(vzConnPtr privconn);
void prlsdkDisconnect(vzConnPtr privconn);
int
prlsdkLoadDomains(vzConnPtr privconn);
-virDomainObjPtr
-prlsdkAddDomain(vzConnPtr privconn, const unsigned char *uuid);
int prlsdkUpdateDomain(vzConnPtr privconn, virDomainObjPtr dom);
+virDomainObjPtr
+prlsdkNewDomain(vzConnPtr privconn,
+ char *name,
+ const unsigned char *uuid);
+int
+prlsdkLoadDomain(vzConnPtr privconn,
+ virDomainObjPtr dom);
int prlsdkSubscribeToPCSEvents(vzConnPtr privconn);
void prlsdkUnsubscribeFromPCSEvents(vzConnPtr privconn);
PRL_RESULT prlsdkStart(PRL_HANDLE sdkdom);
--
1.8.3.1
2:06 a.m.
New subject: [libvirt] [PATCH 4/4] vz: fix race condition when adding domain to domains list
On 28.01.2016 18:38, Mikhail Feoktistov wrote:
Race condition:
User calls defineXML to create new instance.
The main thread from vzDomainDefineXMLFlags() creates new instance by prlsdkCreateVm.
Then this thread calls prlsdkAddDomain to add new domain to domains list.
The second thread receives notification from hypervisor that new VM was created.
It calls prlsdkHandleVmAddedEvent() and also tries to add new domain to domains list.
These two threads call virDomainObjListFindByUUID() from prlsdkAddDomain() and don't
find new domain.
So they add two domains with the same uuid to domains list.
This fix splits logic of prlsdkAddDomain() into two functions.
1. prlsdkNewDomain() creates new empty domain in domains list with the specific uuid.
2. prlsdkLoadDomain() add data from VM to domain object.
New algorithm for creating an instance:
In vzDomainDefineXMLFlags() we add new domain to domain list by calling
prlsdkNewDomain()
and only after that we call CreateVm() to create VM.
It means that we "reserve" domain object with the specific uuid.
After creation of new VM we add info from this VM
to reserved domain object by calling prlsdkLoadDomain().
Before this patch prlsdkLoadDomain() worked in 2 different cases:
1. It creates and initializes new domain. Then updates it from sdk handle.
2. It updates existed domain from sdk handle.
In this patch we remove code which creates new domain from LoadDomain()
and move it to prlsdkNewDomain().
Now prlsdkLoadDomain() only updates domain from skd handle.
In notification handler prlsdkHandleVmAddedEvent() we check
the existence of a domain and if it doesn't exist we add new domain by calling
prlsdkNewDomain() and load info from sdk handle via prlsdkLoadDomain().
---
src/vz/vz_driver.c | 13 ++-
src/vz/vz_sdk.c | 253 +++++++++++++++++++++++++++--------------------------
src/vz/vz_sdk.h | 9 +-
3 files changed, 146 insertions(+), 129 deletions(-)
diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 91a48b6..521efd4 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -685,6 +685,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned
int flags)
virDomainPtr retdom = NULL;
virDomainDefPtr def;
virDomainObjPtr olddom = NULL;
+ virDomainObjPtr newdom = NULL;
unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
@@ -700,6 +701,9 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned
int flags)
olddom = virDomainObjListFindByUUID(privconn->domains, def->uuid);
if (olddom == NULL) {
virResetLastError();
+ newdom = prlsdkNewDomain(privconn, def->name, def->uuid);
+ if (!newdom)
+ goto cleanup;
if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
if (prlsdkCreateVm(conn, def))
goto cleanup;
@@ -713,8 +717,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned
int flags)
goto cleanup;
}
- olddom = prlsdkAddDomain(privconn, def->uuid);
- if (!olddom)
+ if (prlsdkLoadDomain(privconn, newdom))
goto cleanup;
} else {
int state, reason;
@@ -755,6 +758,12 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned
int flags)
cleanup:
if (olddom)
virObjectUnlock(olddom);
+ if (newdom) {
+ if (!retdom)
+ virDomainObjListRemove(privconn->domains, newdom);
+ else
+ virObjectUnlock(newdom);
+ }
virDomainDefFree(def);
vzDriverUnlock(privconn);
return retdom;
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 6fb2a97..9d2bdab 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -1236,58 +1236,35 @@ prlsdkConvertCpuMode(PRL_HANDLE sdkdom, virDomainDefPtr def)
return -1;
}
-/*
- * This function retrieves information about domain.
- * If the domains is already in the domains list
- * privconn->domains, then locked 'olddom' must be
- * provided. If the domains must be added to the list,
- * olddom must be NULL.
- *
- * The function return a pointer to a locked virDomainObj.
- */
-static virDomainObjPtr
-prlsdkLoadDomain(vzConnPtr privconn,
- PRL_HANDLE sdkdom,
- virDomainObjPtr olddom)
+int
+prlsdkLoadDomain(vzConnPtr privconn, virDomainObjPtr dom)
{
- virDomainObjPtr dom = NULL;
virDomainDefPtr def = NULL;
- vzDomObjPtr pdom = NULL;
VIRTUAL_MACHINE_STATE domainState;
+ char *home = NULL;
PRL_UINT32 buflen = 0;
PRL_RESULT pret;
PRL_UINT32 ram;
PRL_UINT32 envId;
PRL_VM_AUTOSTART_OPTION autostart;
+ PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
+ vzDomObjPtr pdom = dom->privateData;
virCheckNonNullArgGoto(privconn, error);
- virCheckNonNullArgGoto(sdkdom, error);
+ virCheckNonNullArgGoto(dom, error);
+
+ sdkdom = prlsdkSdkDomainLookupByUUID(privconn, dom->def->uuid);
+ if (sdkdom == PRL_INVALID_HANDLE)
+ return -1;
if (!(def = virDomainDefNew()))
goto error;
- if (!olddom) {
- if (VIR_ALLOC(pdom) < 0)
- goto error;
- pdom->cache.stats = PRL_INVALID_HANDLE;
- pdom->cache.count = -1;
- if (virCondInit(&pdom->cache.cond) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot
initialize condition"));
- goto error;
- }
- } else {
- pdom = olddom->privateData;
- }
+ def->virtType = dom->def->virtType;
+ def->id = dom->def->id;
- if (STREQ(privconn->drivername, "vz"))
- def->virtType = VIR_DOMAIN_VIRT_VZ;
- else
- def->virtType = VIR_DOMAIN_VIRT_PARALLELS;
-
- def->id = -1;
-
- if (prlsdkGetDomainIds(sdkdom, &def->name, def->uuid) < 0)
+ if (prlsdkGetDomainIds(sdkdom, &def->name, def->uuid))
better stay
with < 0
goto error;
def->onReboot = VIR_DOMAIN_LIFECYCLE_RESTART;
@@ -1318,29 +1295,33 @@ prlsdkLoadDomain(vzConnPtr privconn,
pret = PrlVmCfg_GetEnvId(sdkdom, &envId);
prlsdkCheckRetGoto(pret, error);
- pdom->id = envId;
buflen = 0;
pret = PrlVmCfg_GetHomePath(sdkdom, NULL, &buflen);
prlsdkCheckRetGoto(pret, error);
- VIR_FREE(pdom->home);
- if (VIR_ALLOC_N(pdom->home, buflen) < 0)
+ if (VIR_ALLOC_N(home, buflen) < 0)
goto error;
- pret = PrlVmCfg_GetHomePath(sdkdom, pdom->home, &buflen);
+ pret = PrlVmCfg_GetHomePath(sdkdom, home, &buflen);
prlsdkCheckRetGoto(pret, error);
- /* For VMs pdom->home is actually /directory/config.pvs */
+ /* For VMs home is actually /directory/config.pvs */
if (!IS_CT(def)) {
/* Get rid of /config.pvs in path string */
- char *s = strrchr(pdom->home, '/');
+ char *s = strrchr(home, '/');
if (s)
*s = '\0';
}
pret = PrlVmCfg_GetAutoStart(sdkdom, &autostart);
prlsdkCheckRetGoto(pret, error);
+ if (autostart != PAO_VM_START_ON_LOAD &&
+ autostart != PAO_VM_START_MANUAL) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unknown autostart mode: %X"), autostart);
+ goto error;
+ }
if (prlsdkGetDomainState(sdkdom, &domainState) < 0)
goto error;
@@ -1363,38 +1344,6 @@ prlsdkLoadDomain(vzConnPtr privconn,
goto error;
}
- if (olddom) {
- /* assign new virDomainDef without any checks */
- /* we can't use virDomainObjAssignDef, because it checks
- * for state and domain name */
- dom = olddom;
- virDomainDefFree(dom->def);
- dom->def = def;
- } else {
- if (!(dom = virDomainObjListAdd(privconn->domains, def,
- privconn->xmlopt,
- 0, NULL)))
- goto error;
- }
- /* dom is locked here */
-
- dom->privateData = pdom;
- dom->privateDataFreeFunc = prlsdkDomObjFreePrivate;
- dom->persistent = 1;
-
- switch (autostart) {
- case PAO_VM_START_ON_LOAD:
- dom->autostart = 1;
- break;
- case PAO_VM_START_MANUAL:
- dom->autostart = 0;
- break;
- default:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unknown autostart mode: %X"), autostart);
- goto error;
- }
-
if (prlsdkConvertDomainState(domainState, envId, dom) < 0)
goto error;
@@ -1404,24 +1353,27 @@ prlsdkLoadDomain(vzConnPtr privconn,
pdom->sdkdom = sdkdom;
}
- return dom;
- error:
- if (dom && !olddom) {
- /* Domain isn't persistent means that we haven't yet set
- * prlsdkDomObjFreePrivate and should call it manually
- */
- if (!dom->persistent)
- prlsdkDomObjFreePrivate(pdom);
+ /* assign new virDomainDef without any checks
+ * we can't use virDomainObjAssignDef, because it checks
+ * for state and domain name */
+ virDomainDefFree(dom->def);
+ dom->def = def;
+ pdom->id = envId;
+ VIR_FREE(pdom->home);
+ pdom->home = home;
So you are trying to be atomic here and move all dom and
pdom
changes to place when nothing could go wrong. It's great
but this mixes aims of this patch. It is better move it to
another preparation step(s) so final split of load would
be (mostly) deleting lines from load and move it to another place.
Also look closely on above call of prlsdkConvertDomainState.
It mutates domain too. Looks like it can not fail for runtime reasons
only for code inconsitencies. Can we make it return void?
- virDomainObjListRemove(privconn->domains, dom);
- }
- /* Delete newly allocated def only if we haven't assigned it to domain
- * Otherwise we will end up with domain having invalid def within it
- */
- if (!dom)
- virDomainDefFree(def);
+ if (autostart == PAO_VM_START_ON_LOAD)
+ dom->autostart = 1;
+ else
+ dom->autostart = 0;
- return NULL;
+ PrlHandle_Free(sdkdom);
+ return 0;
+ error:
+ PrlHandle_Free(sdkdom);
+ VIR_FREE(home);
+ virDomainDefFree(def);
+ return -1;
}
int
@@ -1429,11 +1381,13 @@ prlsdkLoadDomains(vzConnPtr privconn)
{
PRL_HANDLE job = PRL_INVALID_HANDLE;
PRL_HANDLE result;
- PRL_HANDLE sdkdom;
+ PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
PRL_UINT32 paramsCount;
PRL_RESULT pret;
size_t i = 0;
virDomainObjPtr dom;
+ unsigned char uuid[VIR_UUID_BUFLEN];
+ char *name = NULL;
job = PrlSrv_GetVmListEx(privconn->server, PVTF_VM | PVTF_CT);
@@ -1447,61 +1401,89 @@ prlsdkLoadDomains(vzConnPtr privconn)
pret = PrlResult_GetParamByIndex(result, i, &sdkdom);
if (PRL_FAILED(pret)) {
logPrlError(pret);
- PrlHandle_Free(sdkdom);
goto error;
}
- dom = prlsdkLoadDomain(privconn, sdkdom, NULL);
- PrlHandle_Free(sdkdom);
+ if (prlsdkGetDomainIds(sdkdom, &name, uuid))
+ goto error;
better consitently use < 0
- if (!dom)
+ if (!(dom = prlsdkNewDomain(privconn, name, uuid)))
goto error;
- else
- virObjectUnlock(dom);
+
+ if (prlsdkLoadDomain(privconn, dom)) {
here too
+ virDomainObjListRemove(privconn->domains, dom);
+ goto error;
+ }
+
+ VIR_FREE(name);
+ PrlHandle_Free(sdkdom);
+ sdkdom = PRL_INVALID_HANDLE;
+ virObjectUnlock(dom);
}
PrlHandle_Free(result);
return 0;
error:
Similar cleanup here and in the loop. May be better to move loop body
into a function.
+ VIR_FREE(name);
+ PrlHandle_Free(sdkdom);
PrlHandle_Free(result);
return -1;
}
-virDomainObjPtr
-prlsdkAddDomain(vzConnPtr privconn, const unsigned char *uuid)
-{
- PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
- virDomainObjPtr dom;
-
- dom = virDomainObjListFindByUUID(privconn->domains, uuid);
- if (dom) {
- /* domain is already in the list */
- return dom;
- }
-
- sdkdom = prlsdkSdkDomainLookupByUUID(privconn, uuid);
- if (sdkdom == PRL_INVALID_HANDLE)
- return NULL;
-
- dom = prlsdkLoadDomain(privconn, sdkdom, NULL);
- PrlHandle_Free(sdkdom);
- return dom;
-}
-
int
prlsdkUpdateDomain(vzConnPtr privconn, virDomainObjPtr dom)
{
PRL_HANDLE job;
- virDomainObjPtr retdom = NULL;
vzDomObjPtr pdom = dom->privateData;
job = PrlVm_RefreshConfig(pdom->sdkdom);
if (waitJob(job))
return -1;
- retdom = prlsdkLoadDomain(privconn, pdom->sdkdom, dom);
- return retdom ? 0 : -1;
+ return prlsdkLoadDomain(privconn, dom);
+}
+
+
+virDomainObjPtr
+prlsdkNewDomain(vzConnPtr privconn, char *name, const unsigned char *uuid)
+{
+ virDomainDefPtr def = NULL;
+ virDomainObjPtr dom = NULL;
+ vzDomObjPtr pdom = NULL;
+
+ if (!(def = virDomainDefNewFull(name, uuid, -1)))
+ goto error;
+
+ if (VIR_ALLOC(pdom) < 0)
+ goto error;
+
+ pdom->cache.stats = PRL_INVALID_HANDLE;
+ pdom->cache.count = -1;
+ if (virCondInit(&pdom->cache.cond) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize
condition"));
+ goto error;
+ }
+
+ if (STREQ(privconn->drivername, "vz"))
+ def->virtType = VIR_DOMAIN_VIRT_VZ;
+ else
+ def->virtType = VIR_DOMAIN_VIRT_PARALLELS;
are you lose def->id = -1
from original code?
+
+ if (!(dom = virDomainObjListAdd(privconn->domains, def,
+ privconn->xmlopt,
+ 0, NULL)))
+ goto error;
+
+ dom->privateData = pdom;
+ dom->privateDataFreeFunc = prlsdkDomObjFreePrivate;
+ dom->persistent = 1;
+ return dom;
+
+ error:
we need to destroy cond.here too. I know it was not in original code))
+ virDomainDefFree(def);
+ VIR_FREE(pdom);
+ return NULL;
}
static int prlsdkSendEvent(vzConnPtr privconn,
@@ -1618,15 +1600,36 @@ prlsdkHandleVmAddedEvent(vzConnPtr privconn,
unsigned char *uuid)
{
virDomainObjPtr dom = NULL;
+ PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
+ char *name = NULL;
- dom = prlsdkAddDomain(privconn, uuid);
- if (dom == NULL)
- return;
+ dom = virDomainObjListFindByUUID(privconn->domains, uuid);
+ if (!dom) {
+ sdkdom = prlsdkSdkDomainLookupByUUID(privconn, uuid);
+ if (sdkdom == PRL_INVALID_HANDLE)
+ goto cleanup;
+
+ if (prlsdkGetDomainIds(sdkdom, &name, NULL))
+ goto cleanup;
+
+ if (!(dom = prlsdkNewDomain(privconn, name, uuid)))
+ goto cleanup;
+
+ if (prlsdkLoadDomain(privconn, dom)) {
+ virDomainObjListRemove(privconn->domains, dom);
+ dom = NULL;
+ goto cleanup;
+ }
this is the exact succession of functions as in load domains, so
another
reason to move it to a distinct function. And then we don't
need the patch that enhance prlsdkGetDomainIds, just ask
for uuid again.
+ }
prlsdkSendEvent(privconn, dom, VIR_DOMAIN_EVENT_DEFINED,
VIR_DOMAIN_EVENT_DEFINED_ADDED);
- virObjectUnlock(dom);
+ cleanup:
+ if (dom)
+ virObjectUnlock(dom);
+ VIR_FREE(name);
+ PrlHandle_Free(sdkdom);
return;
}
diff --git a/src/vz/vz_sdk.h b/src/vz/vz_sdk.h
index ff6be07..f2d3e35 100644
--- a/src/vz/vz_sdk.h
+++ b/src/vz/vz_sdk.h
@@ -30,9 +30,14 @@ int prlsdkConnect(vzConnPtr privconn);
void prlsdkDisconnect(vzConnPtr privconn);
int
prlsdkLoadDomains(vzConnPtr privconn);
-virDomainObjPtr
-prlsdkAddDomain(vzConnPtr privconn, const unsigned char *uuid);
int prlsdkUpdateDomain(vzConnPtr privconn, virDomainObjPtr dom);
+virDomainObjPtr
+prlsdkNewDomain(vzConnPtr privconn,
+ char *name,
+ const unsigned char *uuid);
+int
+prlsdkLoadDomain(vzConnPtr privconn,
+ virDomainObjPtr dom);
int prlsdkSubscribeToPCSEvents(vzConnPtr privconn);
void prlsdkUnsubscribeFromPCSEvents(vzConnPtr privconn);
PRL_RESULT prlsdkStart(PRL_HANDLE sdkdom);
Overall this patch is too big.
10:38 a.m.
New subject: [libvirt] [PATCH 4/4] vz: fix race condition when adding domain to domains list
29.01.2016 11:06, Nikolay Shirokovskiy пишет:
On 28.01.2016 18:38, Mikhail Feoktistov wrote:
> Race condition:
> User calls defineXML to create new instance.
> The main thread from vzDomainDefineXMLFlags() creates new instance by
prlsdkCreateVm.
> Then this thread calls prlsdkAddDomain to add new domain to domains list.
> The second thread receives notification from hypervisor that new VM was created.
> It calls prlsdkHandleVmAddedEvent() and also tries to add new domain to domains
list.
> These two threads call virDomainObjListFindByUUID() from prlsdkAddDomain() and
don't find new domain.
> So they add two domains with the same uuid to domains list.
>
> This fix splits logic of prlsdkAddDomain() into two functions.
> 1. prlsdkNewDomain() creates new empty domain in domains list with the specific
uuid.
> 2. prlsdkLoadDomain() add data from VM to domain object.
>
> New algorithm for creating an instance:
> In vzDomainDefineXMLFlags() we add new domain to domain list by calling
prlsdkNewDomain()
> and only after that we call CreateVm() to create VM.
> It means that we "reserve" domain object with the specific uuid.
> After creation of new VM we add info from this VM
> to reserved domain object by calling prlsdkLoadDomain().
>
> Before this patch prlsdkLoadDomain() worked in 2 different cases:
> 1. It creates and initializes new domain. Then updates it from sdk handle.
> 2. It updates existed domain from sdk handle.
> In this patch we remove code which creates new domain from LoadDomain()
> and move it to prlsdkNewDomain().
> Now prlsdkLoadDomain() only updates domain from skd handle.
>
> In notification handler prlsdkHandleVmAddedEvent() we check
> the existence of a domain and if it doesn't exist we add new domain by calling
> prlsdkNewDomain() and load info from sdk handle via prlsdkLoadDomain().
> ---
> src/vz/vz_driver.c | 13 ++-
> src/vz/vz_sdk.c | 253 +++++++++++++++++++++++++++--------------------------
> src/vz/vz_sdk.h | 9 +-
> 3 files changed, 146 insertions(+), 129 deletions(-)
>
> diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
> index 91a48b6..521efd4 100644
> --- a/src/vz/vz_driver.c
> +++ b/src/vz/vz_driver.c
> @@ -685,6 +685,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flags)
> virDomainPtr retdom = NULL;
> virDomainDefPtr def;
> virDomainObjPtr olddom = NULL;
> + virDomainObjPtr newdom = NULL;
> unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
>
> virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
> @@ -700,6 +701,9 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flags)
> olddom = virDomainObjListFindByUUID(privconn->domains, def->uuid);
> if (olddom == NULL) {
> virResetLastError();
> + newdom = prlsdkNewDomain(privconn, def->name, def->uuid);
> + if (!newdom)
> + goto cleanup;
> if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
> if (prlsdkCreateVm(conn, def))
> goto cleanup;
> @@ -713,8 +717,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flags)
> goto cleanup;
> }
>
> - olddom = prlsdkAddDomain(privconn, def->uuid);
> - if (!olddom)
> + if (prlsdkLoadDomain(privconn, newdom))
> goto cleanup;
> } else {
> int state, reason;
> @@ -755,6 +758,12 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml,
unsigned int flags)
> cleanup:
> if (olddom)
> virObjectUnlock(olddom);
> + if (newdom) {
> + if (!retdom)
> + virDomainObjListRemove(privconn->domains, newdom);
> + else
> + virObjectUnlock(newdom);
> + }
> virDomainDefFree(def);
> vzDriverUnlock(privconn);
> return retdom;
> diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
> index 6fb2a97..9d2bdab 100644
> --- a/src/vz/vz_sdk.c
> +++ b/src/vz/vz_sdk.c
> @@ -1236,58 +1236,35 @@ prlsdkConvertCpuMode(PRL_HANDLE sdkdom, virDomainDefPtr def)
> return -1;
> }
>
> -/*
> - * This function retrieves information about domain.
> - * If the domains is already in the domains list
> - * privconn->domains, then locked 'olddom' must be
> - * provided. If the domains must be added to the list,
> - * olddom must be NULL.
> - *
> - * The function return a pointer to a locked virDomainObj.
> - */
> -static virDomainObjPtr
> -prlsdkLoadDomain(vzConnPtr privconn,
> - PRL_HANDLE sdkdom,
> - virDomainObjPtr olddom)
> +int
> +prlsdkLoadDomain(vzConnPtr privconn, virDomainObjPtr dom)
> {
> - virDomainObjPtr dom = NULL;
> virDomainDefPtr def = NULL;
> - vzDomObjPtr pdom = NULL;
> VIRTUAL_MACHINE_STATE domainState;
> + char *home = NULL;
>
> PRL_UINT32 buflen = 0;
> PRL_RESULT pret;
> PRL_UINT32 ram;
> PRL_UINT32 envId;
> PRL_VM_AUTOSTART_OPTION autostart;
> + PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
> + vzDomObjPtr pdom = dom->privateData;
>
> virCheckNonNullArgGoto(privconn, error);
> - virCheckNonNullArgGoto(sdkdom, error);
> + virCheckNonNullArgGoto(dom, error);
> +
> + sdkdom = prlsdkSdkDomainLookupByUUID(privconn, dom->def->uuid);
> + if (sdkdom == PRL_INVALID_HANDLE)
> + return -1;
>
> if (!(def = virDomainDefNew()))
> goto error;
>
> - if (!olddom) {
> - if (VIR_ALLOC(pdom) < 0)
> - goto error;
> - pdom->cache.stats = PRL_INVALID_HANDLE;
> - pdom->cache.count = -1;
> - if (virCondInit(&pdom->cache.cond) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot
initialize condition"));
> - goto error;
> - }
> - } else {
> - pdom = olddom->privateData;
> - }
> + def->virtType = dom->def->virtType;
> + def->id = dom->def->id;
>
> - if (STREQ(privconn->drivername, "vz"))
> - def->virtType = VIR_DOMAIN_VIRT_VZ;
> - else
> - def->virtType = VIR_DOMAIN_VIRT_PARALLELS;
> -
> - def->id = -1;
> -
> - if (prlsdkGetDomainIds(sdkdom, &def->name, def->uuid) < 0)
> + if (prlsdkGetDomainIds(sdkdom, &def->name, def->uuid))
better stay with < 0
Ok
> goto error;
>
> def->onReboot = VIR_DOMAIN_LIFECYCLE_RESTART;
> @@ -1318,29 +1295,33 @@ prlsdkLoadDomain(vzConnPtr privconn,
>
> pret = PrlVmCfg_GetEnvId(sdkdom, &envId);
> prlsdkCheckRetGoto(pret, error);
> - pdom->id = envId;
>
> buflen = 0;
> pret = PrlVmCfg_GetHomePath(sdkdom, NULL, &buflen);
> prlsdkCheckRetGoto(pret, error);
>
> - VIR_FREE(pdom->home);
> - if (VIR_ALLOC_N(pdom->home, buflen) < 0)
> + if (VIR_ALLOC_N(home, buflen) < 0)
> goto error;
>
> - pret = PrlVmCfg_GetHomePath(sdkdom, pdom->home, &buflen);
> + pret = PrlVmCfg_GetHomePath(sdkdom, home, &buflen);
> prlsdkCheckRetGoto(pret, error);
>
> - /* For VMs pdom->home is actually /directory/config.pvs */
> + /* For VMs home is actually /directory/config.pvs */
> if (!IS_CT(def)) {
> /* Get rid of /config.pvs in path string */
> - char *s = strrchr(pdom->home, '/');
> + char *s = strrchr(home, '/');
> if (s)
> *s = '\0';
> }
>
> pret = PrlVmCfg_GetAutoStart(sdkdom, &autostart);
> prlsdkCheckRetGoto(pret, error);
> + if (autostart != PAO_VM_START_ON_LOAD &&
> + autostart != PAO_VM_START_MANUAL) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unknown autostart mode: %X"), autostart);
> + goto error;
> + }
>
> if (prlsdkGetDomainState(sdkdom, &domainState) < 0)
> goto error;
> @@ -1363,38 +1344,6 @@ prlsdkLoadDomain(vzConnPtr privconn,
> goto error;
> }
>
> - if (olddom) {
> - /* assign new virDomainDef without any checks */
> - /* we can't use virDomainObjAssignDef, because it checks
> - * for state and domain name */
> - dom = olddom;
> - virDomainDefFree(dom->def);
> - dom->def = def;
> - } else {
> - if (!(dom = virDomainObjListAdd(privconn->domains, def,
> - privconn->xmlopt,
> - 0, NULL)))
> - goto error;
> - }
> - /* dom is locked here */
> -
> - dom->privateData = pdom;
> - dom->privateDataFreeFunc = prlsdkDomObjFreePrivate;
> - dom->persistent = 1;
> -
> - switch (autostart) {
> - case PAO_VM_START_ON_LOAD:
> - dom->autostart = 1;
> - break;
> - case PAO_VM_START_MANUAL:
> - dom->autostart = 0;
> - break;
> - default:
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Unknown autostart mode: %X"), autostart);
> - goto error;
> - }
> -
> if (prlsdkConvertDomainState(domainState, envId, dom) < 0)
> goto error;
>
> @@ -1404,24 +1353,27 @@ prlsdkLoadDomain(vzConnPtr privconn,
> pdom->sdkdom = sdkdom;
> }
>
> - return dom;
> - error:
> - if (dom && !olddom) {
> - /* Domain isn't persistent means that we haven't yet set
> - * prlsdkDomObjFreePrivate and should call it manually
> - */
> - if (!dom->persistent)
> - prlsdkDomObjFreePrivate(pdom);
> + /* assign new virDomainDef without any checks
> + * we can't use virDomainObjAssignDef, because it checks
> + * for state and domain name */
> + virDomainDefFree(dom->def);
> + dom->def = def;
> + pdom->id = envId;
> + VIR_FREE(pdom->home);
> + pdom->home = home;
So you are trying to be atomic here and move all dom and pdom
changes to place when nothing could go wrong. It's great
but this mixes aims of this patch. It is better move it to
another preparation step(s) so final split of load would
be (mostly) deleting lines from load and move it to another place.
Also look closely on above call of prlsdkConvertDomainState.
It mutates domain too. Looks like it can not fail for runtime reasons
only for code inconsitencies. Can we make it return void?
Keep
prlsdkConvertDomainState as is, but I change the sequence of calls
in prlsdkLoadDomain
see details in new version of this patch
>
> - virDomainObjListRemove(privconn->domains, dom);
> - }
> - /* Delete newly allocated def only if we haven't assigned it to domain
> - * Otherwise we will end up with domain having invalid def within it
> - */
> - if (!dom)
> - virDomainDefFree(def);
> + if (autostart == PAO_VM_START_ON_LOAD)
> + dom->autostart = 1;
> + else
> + dom->autostart = 0;
>
> - return NULL;
> + PrlHandle_Free(sdkdom);
> + return 0;
> + error:
> + PrlHandle_Free(sdkdom);
> + VIR_FREE(home);
> + virDomainDefFree(def);
> + return -1;
> }
>
> int
> @@ -1429,11 +1381,13 @@ prlsdkLoadDomains(vzConnPtr privconn)
> {
> PRL_HANDLE job = PRL_INVALID_HANDLE;
> PRL_HANDLE result;
> - PRL_HANDLE sdkdom;
> + PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
> PRL_UINT32 paramsCount;
> PRL_RESULT pret;
> size_t i = 0;
> virDomainObjPtr dom;
> + unsigned char uuid[VIR_UUID_BUFLEN];
> + char *name = NULL;
>
> job = PrlSrv_GetVmListEx(privconn->server, PVTF_VM | PVTF_CT);
>
> @@ -1447,61 +1401,89 @@ prlsdkLoadDomains(vzConnPtr privconn)
> pret = PrlResult_GetParamByIndex(result, i, &sdkdom);
> if (PRL_FAILED(pret)) {
> logPrlError(pret);
> - PrlHandle_Free(sdkdom);
> goto error;
> }
>
> - dom = prlsdkLoadDomain(privconn, sdkdom, NULL);
> - PrlHandle_Free(sdkdom);
> + if (prlsdkGetDomainIds(sdkdom, &name, uuid))
> + goto error;
better consitently use < 0
Ok
>
> - if (!dom)
> + if (!(dom = prlsdkNewDomain(privconn, name, uuid)))
> goto error;
> - else
> - virObjectUnlock(dom);
> +
> + if (prlsdkLoadDomain(privconn, dom)) {
here too
Ok
> + virDomainObjListRemove(privconn->domains, dom);
> + goto error;
> + }
> +
> + VIR_FREE(name);
> + PrlHandle_Free(sdkdom);
> + sdkdom = PRL_INVALID_HANDLE;
> + virObjectUnlock(dom);
> }
>
> PrlHandle_Free(result);
> return 0;
>
> error:
Similar cleanup here and in the loop. May be better to move loop body
into a function.
move this code to new func prlsdkNewDomainByHandle
> + VIR_FREE(name);
> + PrlHandle_Free(sdkdom);
> PrlHandle_Free(result);
> return -1;
> }
>
> -virDomainObjPtr
> -prlsdkAddDomain(vzConnPtr privconn, const unsigned char *uuid)
> -{
> - PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
> - virDomainObjPtr dom;
> -
> - dom = virDomainObjListFindByUUID(privconn->domains, uuid);
> - if (dom) {
> - /* domain is already in the list */
> - return dom;
> - }
> -
> - sdkdom = prlsdkSdkDomainLookupByUUID(privconn, uuid);
> - if (sdkdom == PRL_INVALID_HANDLE)
> - return NULL;
> -
> - dom = prlsdkLoadDomain(privconn, sdkdom, NULL);
> - PrlHandle_Free(sdkdom);
> - return dom;
> -}
> -
> int
> prlsdkUpdateDomain(vzConnPtr privconn, virDomainObjPtr dom)
> {
> PRL_HANDLE job;
> - virDomainObjPtr retdom = NULL;
> vzDomObjPtr pdom = dom->privateData;
>
> job = PrlVm_RefreshConfig(pdom->sdkdom);
> if (waitJob(job))
> return -1;
>
> - retdom = prlsdkLoadDomain(privconn, pdom->sdkdom, dom);
> - return retdom ? 0 : -1;
> + return prlsdkLoadDomain(privconn, dom);
> +}
> +
> +
> +virDomainObjPtr
> +prlsdkNewDomain(vzConnPtr privconn, char *name, const unsigned char *uuid)
> +{
> + virDomainDefPtr def = NULL;
> + virDomainObjPtr dom = NULL;
> + vzDomObjPtr pdom = NULL;
> +
> + if (!(def = virDomainDefNewFull(name, uuid, -1)))
> + goto error;
> +
> + if (VIR_ALLOC(pdom) < 0)
> + goto error;
> +
> + pdom->cache.stats = PRL_INVALID_HANDLE;
> + pdom->cache.count = -1;
> + if (virCondInit(&pdom->cache.cond) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot
initialize condition"));
> + goto error;
> + }
> +
> + if (STREQ(privconn->drivername, "vz"))
> + def->virtType = VIR_DOMAIN_VIRT_VZ;
> + else
> + def->virtType = VIR_DOMAIN_VIRT_PARALLELS;
are you lose def->id = -1 from original code?
We set def->id = -1 in
virDomainDefNewFull(... -1) the last argument.
> +
> + if (!(dom = virDomainObjListAdd(privconn->domains, def,
> + privconn->xmlopt,
> + 0, NULL)))
> + goto error;
> +
> + dom->privateData = pdom;
> + dom->privateDataFreeFunc = prlsdkDomObjFreePrivate;
> + dom->persistent = 1;
> + return dom;
> +
> + error:
we need to destroy cond.here too. I know it was not in original code))
Ok
> + virDomainDefFree(def);
> + VIR_FREE(pdom);
> + return NULL;
> }
>
> static int prlsdkSendEvent(vzConnPtr privconn,
> @@ -1618,15 +1600,36 @@ prlsdkHandleVmAddedEvent(vzConnPtr privconn,
> unsigned char *uuid)
> {
> virDomainObjPtr dom = NULL;
> + PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
> + char *name = NULL;
>
> - dom = prlsdkAddDomain(privconn, uuid);
> - if (dom == NULL)
> - return;
> + dom = virDomainObjListFindByUUID(privconn->domains, uuid);
> + if (!dom) {
> + sdkdom = prlsdkSdkDomainLookupByUUID(privconn, uuid);
> + if (sdkdom == PRL_INVALID_HANDLE)
> + goto cleanup;
> +
> + if (prlsdkGetDomainIds(sdkdom, &name, NULL))
> + goto cleanup;
> +
> + if (!(dom = prlsdkNewDomain(privconn, name, uuid)))
> + goto cleanup;
> +
> + if (prlsdkLoadDomain(privconn, dom)) {
> + virDomainObjListRemove(privconn->domains, dom);
> + dom = NULL;
> + goto cleanup;
> + }
this is the exact succession of functions as in load domains, so another
reason to move it to a distinct function. And then we don't
need the patch that enhance prlsdkGetDomainIds, just ask
for uuid again.
Done
> + }
>
> prlsdkSendEvent(privconn, dom, VIR_DOMAIN_EVENT_DEFINED,
> VIR_DOMAIN_EVENT_DEFINED_ADDED);
>
> - virObjectUnlock(dom);
> + cleanup:
> + if (dom)
> + virObjectUnlock(dom);
> + VIR_FREE(name);
> + PrlHandle_Free(sdkdom);
> return;
> }
>
> diff --git a/src/vz/vz_sdk.h b/src/vz/vz_sdk.h
> index ff6be07..f2d3e35 100644
> --- a/src/vz/vz_sdk.h
> +++ b/src/vz/vz_sdk.h
> @@ -30,9 +30,14 @@ int prlsdkConnect(vzConnPtr privconn);
> void prlsdkDisconnect(vzConnPtr privconn);
> int
> prlsdkLoadDomains(vzConnPtr privconn);
> -virDomainObjPtr
> -prlsdkAddDomain(vzConnPtr privconn, const unsigned char *uuid);
> int prlsdkUpdateDomain(vzConnPtr privconn, virDomainObjPtr dom);
> +virDomainObjPtr
> +prlsdkNewDomain(vzConnPtr privconn,
> + char *name,
> + const unsigned char *uuid);
> +int
> +prlsdkLoadDomain(vzConnPtr privconn,
> + virDomainObjPtr dom);
> int prlsdkSubscribeToPCSEvents(vzConnPtr privconn);
> void prlsdkUnsubscribeFromPCSEvents(vzConnPtr privconn);
> PRL_RESULT prlsdkStart(PRL_HANDLE sdkdom);
>
Overall this patch is too big.
8:39 a.m.
New subject: [libvirt] [PATCH 4/4] vz: fix race condition when adding domain to domains list
28.01.2016 18:38, Mikhail Feoktistov пишет:
Race condition:
User calls defineXML to create new instance.
The main thread from vzDomainDefineXMLFlags() creates new instance by prlsdkCreateVm.
Then this thread calls prlsdkAddDomain to add new domain to domains list.
The second thread receives notification from hypervisor that new VM was created.
It calls prlsdkHandleVmAddedEvent() and also tries to add new domain to domains list.
These two threads call virDomainObjListFindByUUID() from prlsdkAddDomain() and don't
find new domain.
So they add two domains with the same uuid to domains list.
This fix splits logic of prlsdkAddDomain() into two functions.
1. prlsdkNewDomain() creates new empty domain in domains list with the specific uuid.
2. prlsdkLoadDomain() add data from VM to domain object.
New algorithm for creating an instance:
In vzDomainDefineXMLFlags() we add new domain to domain list by calling
prlsdkNewDomain()
and only after that we call CreateVm() to create VM.
It means that we "reserve" domain object with the specific uuid.
After creation of new VM we add info from this VM
to reserved domain object by calling prlsdkLoadDomain().
Before this patch prlsdkLoadDomain() worked in 2 different cases:
1. It creates and initializes new domain. Then updates it from sdk handle.
2. It updates existed domain from sdk handle.
In this patch we remove code which creates new domain from LoadDomain()
and move it to prlsdkNewDomain().
Now prlsdkLoadDomain() only updates domain from skd handle.
In notification handler prlsdkHandleVmAddedEvent() we check
the existence of a domain and if it doesn't exist we add new domain by calling
prlsdkNewDomain() and load info from sdk handle via prlsdkLoadDomain().
---
src/vz/vz_driver.c | 13 ++-
src/vz/vz_sdk.c | 253 +++++++++++++++++++++++++++--------------------------
src/vz/vz_sdk.h | 9 +-
3 files changed, 146 insertions(+), 129 deletions(-)
diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 91a48b6..521efd4 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -685,6 +685,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned
int flags)
virDomainPtr retdom = NULL;
virDomainDefPtr def;
virDomainObjPtr olddom = NULL;
+ virDomainObjPtr newdom = NULL;
unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
@@ -700,6 +701,9 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned
int flags)
olddom = virDomainObjListFindByUUID(privconn->domains, def->uuid);
if (olddom == NULL) {
virResetLastError();
+ newdom = prlsdkNewDomain(privconn, def->name, def->uuid);
+ if (!newdom)
+ goto cleanup;
if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
if (prlsdkCreateVm(conn, def))
goto cleanup;
@@ -713,8 +717,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned
int flags)
goto cleanup;
}
- olddom = prlsdkAddDomain(privconn, def->uuid);
- if (!olddom)
+ if (prlsdkLoadDomain(privconn, newdom))
goto cleanup;
} else {
int state, reason;
@@ -755,6 +758,12 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned
int flags)
cleanup:
if (olddom)
virObjectUnlock(olddom);
+ if (newdom) {
+ if (!retdom)
+ virDomainObjListRemove(privconn->domains, newdom);
+ else
+ virObjectUnlock(newdom);
+ }
virDomainDefFree(def);
vzDriverUnlock(privconn);
return retdom;
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 6fb2a97..9d2bdab 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -1236,58 +1236,35 @@ prlsdkConvertCpuMode(PRL_HANDLE sdkdom, virDomainDefPtr def)
return -1;
}
-/*
- * This function retrieves information about domain.
- * If the domains is already in the domains list
- * privconn->domains, then locked 'olddom' must be
- * provided. If the domains must be added to the list,
- * olddom must be NULL.
- *
- * The function return a pointer to a locked virDomainObj.
- */
-static virDomainObjPtr
-prlsdkLoadDomain(vzConnPtr privconn,
- PRL_HANDLE sdkdom,
- virDomainObjPtr olddom)
+int
+prlsdkLoadDomain(vzConnPtr privconn, virDomainObjPtr dom)
{
- virDomainObjPtr dom = NULL;
virDomainDefPtr def = NULL;
- vzDomObjPtr pdom = NULL;
VIRTUAL_MACHINE_STATE domainState;
+ char *home = NULL;
PRL_UINT32 buflen = 0;
PRL_RESULT pret;
PRL_UINT32 ram;
PRL_UINT32 envId;
PRL_VM_AUTOSTART_OPTION autostart;
+ PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
+ vzDomObjPtr pdom = dom->privateData;
virCheckNonNullArgGoto(privconn, error);
- virCheckNonNullArgGoto(sdkdom, error);
+ virCheckNonNullArgGoto(dom, error);
+
+ sdkdom = prlsdkSdkDomainLookupByUUID(privconn, dom->def->uuid);
+ if (sdkdom == PRL_INVALID_HANDLE)
+ return -1;
if (!(def = virDomainDefNew()))
goto error;
- if (!olddom) {
- if (VIR_ALLOC(pdom) < 0)
- goto error;
- pdom->cache.stats = PRL_INVALID_HANDLE;
- pdom->cache.count = -1;
- if (virCondInit(&pdom->cache.cond) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot
initialize condition"));
- goto error;
- }
- } else {
- pdom = olddom->privateData;
- }
+ def->virtType = dom->def->virtType;
+ def->id = dom->def->id;
- if (STREQ(privconn->drivername, "vz"))
- def->virtType = VIR_DOMAIN_VIRT_VZ;
- else
- def->virtType = VIR_DOMAIN_VIRT_PARALLELS;
-
- def->id = -1;
-
- if (prlsdkGetDomainIds(sdkdom, &def->name, def->uuid) < 0)
+ if (prlsdkGetDomainIds(sdkdom, &def->name, def->uuid))
goto error;
def->onReboot = VIR_DOMAIN_LIFECYCLE_RESTART;
@@ -1318,29 +1295,33 @@ prlsdkLoadDomain(vzConnPtr privconn,
pret = PrlVmCfg_GetEnvId(sdkdom, &envId);
prlsdkCheckRetGoto(pret, error);
- pdom->id = envId;
buflen = 0;
pret = PrlVmCfg_GetHomePath(sdkdom, NULL, &buflen);
prlsdkCheckRetGoto(pret, error);
- VIR_FREE(pdom->home);
- if (VIR_ALLOC_N(pdom->home, buflen) < 0)
+ if (VIR_ALLOC_N(home, buflen) < 0)
goto error;
- pret = PrlVmCfg_GetHomePath(sdkdom, pdom->home, &buflen);
+ pret = PrlVmCfg_GetHomePath(sdkdom, home, &buflen);
prlsdkCheckRetGoto(pret, error);
- /* For VMs pdom->home is actually /directory/config.pvs */
+ /* For VMs home is actually /directory/config.pvs */
if (!IS_CT(def)) {
/* Get rid of /config.pvs in path string */
- char *s = strrchr(pdom->home, '/');
+ char *s = strrchr(home, '/');
if (s)
*s = '\0';
}
pret = PrlVmCfg_GetAutoStart(sdkdom, &autostart);
prlsdkCheckRetGoto(pret, error);
+ if (autostart != PAO_VM_START_ON_LOAD &&
+ autostart != PAO_VM_START_MANUAL) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unknown autostart mode: %X"), autostart);
+ goto error;
+ }
if (prlsdkGetDomainState(sdkdom, &domainState) < 0)
goto error;
@@ -1363,38 +1344,6 @@ prlsdkLoadDomain(vzConnPtr privconn,
goto error;
}
- if (olddom) {
- /* assign new virDomainDef without any checks */
- /* we can't use virDomainObjAssignDef, because it checks
- * for state and domain name */
- dom = olddom;
- virDomainDefFree(dom->def);
- dom->def = def;
- } else {
- if (!(dom = virDomainObjListAdd(privconn->domains, def,
- privconn->xmlopt,
- 0, NULL)))
- goto error;
- }
- /* dom is locked here */
-
- dom->privateData = pdom;
- dom->privateDataFreeFunc = prlsdkDomObjFreePrivate;
- dom->persistent = 1;
-
- switch (autostart) {
- case PAO_VM_START_ON_LOAD:
- dom->autostart = 1;
- break;
- case PAO_VM_START_MANUAL:
- dom->autostart = 0;
- break;
- default:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unknown autostart mode: %X"), autostart);
- goto error;
- }
-
if (prlsdkConvertDomainState(domainState, envId, dom) < 0)
goto error;
@@ -1404,24 +1353,27 @@ prlsdkLoadDomain(vzConnPtr privconn,
pdom->sdkdom = sdkdom;
}
- return dom;
- error:
- if (dom && !olddom) {
- /* Domain isn't persistent means that we haven't yet set
- * prlsdkDomObjFreePrivate and should call it manually
- */
- if (!dom->persistent)
- prlsdkDomObjFreePrivate(pdom);
+ /* assign new virDomainDef without any checks
+ * we can't use virDomainObjAssignDef, because it checks
+ * for state and domain name */
+ virDomainDefFree(dom->def);
+ dom->def = def;
+ pdom->id = envId;
+ VIR_FREE(pdom->home);
+ pdom->home = home;
- virDomainObjListRemove(privconn->domains, dom);
- }
- /* Delete newly allocated def only if we haven't assigned it to domain
- * Otherwise we will end up with domain having invalid def within it
- */
- if (!dom)
- virDomainDefFree(def);
+ if (autostart == PAO_VM_START_ON_LOAD)
+ dom->autostart = 1;
+ else
+ dom->autostart = 0;
- return NULL;
+ PrlHandle_Free(sdkdom);
+ return 0;
+ error:
+ PrlHandle_Free(sdkdom);
+ VIR_FREE(home);
+ virDomainDefFree(def);
+ return -1;
}
int
@@ -1429,11 +1381,13 @@ prlsdkLoadDomains(vzConnPtr privconn)
{
PRL_HANDLE job = PRL_INVALID_HANDLE;
PRL_HANDLE result;
- PRL_HANDLE sdkdom;
+ PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
PRL_UINT32 paramsCount;
PRL_RESULT pret;
size_t i = 0;
virDomainObjPtr dom;
+ unsigned char uuid[VIR_UUID_BUFLEN];
+ char *name = NULL;
job = PrlSrv_GetVmListEx(privconn->server, PVTF_VM | PVTF_CT);
@@ -1447,61 +1401,89 @@ prlsdkLoadDomains(vzConnPtr privconn)
pret = PrlResult_GetParamByIndex(result, i, &sdkdom);
if (PRL_FAILED(pret)) {
logPrlError(pret);
- PrlHandle_Free(sdkdom);
goto error;
}
- dom = prlsdkLoadDomain(privconn, sdkdom, NULL);
- PrlHandle_Free(sdkdom);
+ if (prlsdkGetDomainIds(sdkdom, &name, uuid))
+ goto error;
- if (!dom)
+ if (!(dom = prlsdkNewDomain(privconn, name, uuid)))
goto error;
- else
- virObjectUnlock(dom);
+
+ if (prlsdkLoadDomain(privconn, dom)) {
+ virDomainObjListRemove(privconn->domains, dom);
+ goto error;
+ }
+
+ VIR_FREE(name);
+ PrlHandle_Free(sdkdom);
+ sdkdom = PRL_INVALID_HANDLE;
+ virObjectUnlock(dom);
}
PrlHandle_Free(result);
return 0;
error:
+ VIR_FREE(name);
+ PrlHandle_Free(sdkdom);
PrlHandle_Free(result);
return -1;
}
-virDomainObjPtr
-prlsdkAddDomain(vzConnPtr privconn, const unsigned char *uuid)
-{
- PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
- virDomainObjPtr dom;
-
- dom = virDomainObjListFindByUUID(privconn->domains, uuid);
- if (dom) {
- /* domain is already in the list */
- return dom;
- }
-
- sdkdom = prlsdkSdkDomainLookupByUUID(privconn, uuid);
- if (sdkdom == PRL_INVALID_HANDLE)
- return NULL;
-
- dom = prlsdkLoadDomain(privconn, sdkdom, NULL);
- PrlHandle_Free(sdkdom);
- return dom;
-}
-
int
prlsdkUpdateDomain(vzConnPtr privconn, virDomainObjPtr dom)
{
PRL_HANDLE job;
- virDomainObjPtr retdom = NULL;
vzDomObjPtr pdom = dom->privateData;
job = PrlVm_RefreshConfig(pdom->sdkdom);
if (waitJob(job))
return -1;
- retdom = prlsdkLoadDomain(privconn, pdom->sdkdom, dom);
- return retdom ? 0 : -1;
+ return prlsdkLoadDomain(privconn, dom);
+}
+
+
+virDomainObjPtr
+prlsdkNewDomain(vzConnPtr privconn, char *name, const unsigned char *uuid)
+{
Since this function doesn't communicate with prlsdk its name prefix
looks strange to me.
Also I would think about this function to vz_driver.c, but I don't
insist on it.
+ virDomainDefPtr def = NULL;
+ virDomainObjPtr dom = NULL;
+ vzDomObjPtr pdom = NULL;
+
+ if (!(def = virDomainDefNewFull(name, uuid, -1)))
+ goto error;
+
+ if (VIR_ALLOC(pdom) < 0)
+ goto error;
+
+ pdom->cache.stats = PRL_INVALID_HANDLE;
+ pdom->cache.count = -1;
+ if (virCondInit(&pdom->cache.cond) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize
condition"));
+ goto error;
+ }
+
+ if (STREQ(privconn->drivername, "vz"))
+ def->virtType = VIR_DOMAIN_VIRT_VZ;
+ else
+ def->virtType = VIR_DOMAIN_VIRT_PARALLELS;
+
+ if (!(dom = virDomainObjListAdd(privconn->domains, def,
+ privconn->xmlopt,
+ 0, NULL)))
+ goto error;
+
+ dom->privateData = pdom;
+ dom->privateDataFreeFunc = prlsdkDomObjFreePrivate;
+ dom->persistent = 1;
+ return dom;
+
+ error:
+ virDomainDefFree(def);
+ VIR_FREE(pdom);
+ return NULL;
}
static int prlsdkSendEvent(vzConnPtr privconn,
@@ -1618,15 +1600,36 @@ prlsdkHandleVmAddedEvent(vzConnPtr privconn,
unsigned char *uuid)
{
virDomainObjPtr dom = NULL;
+ PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
+ char *name = NULL;
- dom = prlsdkAddDomain(privconn, uuid);
- if (dom == NULL)
- return;
+ dom = virDomainObjListFindByUUID(privconn->domains, uuid);
+ if (!dom) {
+ sdkdom = prlsdkSdkDomainLookupByUUID(privconn, uuid);
+ if (sdkdom == PRL_INVALID_HANDLE)
+ goto cleanup;
+
+ if (prlsdkGetDomainIds(sdkdom, &name, NULL))
+ goto cleanup;
+
+ if (!(dom = prlsdkNewDomain(privconn, name, uuid)))
+ goto cleanup;
+
+ if (prlsdkLoadDomain(privconn, dom)) {
+ virDomainObjListRemove(privconn->domains, dom);
+ dom = NULL;
+ goto cleanup;
+ }
+ }
prlsdkSendEvent(privconn, dom, VIR_DOMAIN_EVENT_DEFINED,
VIR_DOMAIN_EVENT_DEFINED_ADDED);
- virObjectUnlock(dom);
+ cleanup:
+ if (dom)
+ virObjectUnlock(dom);
+ VIR_FREE(name);
+ PrlHandle_Free(sdkdom);
return;
}
diff --git a/src/vz/vz_sdk.h b/src/vz/vz_sdk.h
index ff6be07..f2d3e35 100644
--- a/src/vz/vz_sdk.h
+++ b/src/vz/vz_sdk.h
@@ -30,9 +30,14 @@ int prlsdkConnect(vzConnPtr privconn);
void prlsdkDisconnect(vzConnPtr privconn);
int
prlsdkLoadDomains(vzConnPtr privconn);
-virDomainObjPtr
-prlsdkAddDomain(vzConnPtr privconn, const unsigned char *uuid);
int prlsdkUpdateDomain(vzConnPtr privconn, virDomainObjPtr dom);
+virDomainObjPtr
+prlsdkNewDomain(vzConnPtr privconn,
+ char *name,
+ const unsigned char *uuid);
+int
+prlsdkLoadDomain(vzConnPtr privconn,
+ virDomainObjPtr dom);
int prlsdkSubscribeToPCSEvents(vzConnPtr privconn);
void prlsdkUnsubscribeFromPCSEvents(vzConnPtr privconn);
PRL_RESULT prlsdkStart(PRL_HANDLE sdkdom);
9:23 a.m.
New subject: [libvirt] [PATCH 4/4] vz: fix race condition when adding domain to domains list
29.01.2016 17:39, Maxim Nestratov пишет:
28.01.2016 18:38, Mikhail Feoktistov пишет:
> Race condition:
> User calls defineXML to create new instance.
> The main thread from vzDomainDefineXMLFlags() creates new instance by
> prlsdkCreateVm.
> Then this thread calls prlsdkAddDomain to add new domain to domains
> list.
> The second thread receives notification from hypervisor that new VM
> was created.
> It calls prlsdkHandleVmAddedEvent() and also tries to add new domain
> to domains list.
> These two threads call virDomainObjListFindByUUID() from
> prlsdkAddDomain() and don't find new domain.
> So they add two domains with the same uuid to domains list.
>
> This fix splits logic of prlsdkAddDomain() into two functions.
> 1. prlsdkNewDomain() creates new empty domain in domains list with
> the specific uuid.
> 2. prlsdkLoadDomain() add data from VM to domain object.
>
> New algorithm for creating an instance:
> In vzDomainDefineXMLFlags() we add new domain to domain list by
> calling prlsdkNewDomain()
> and only after that we call CreateVm() to create VM.
> It means that we "reserve" domain object with the specific uuid.
> After creation of new VM we add info from this VM
> to reserved domain object by calling prlsdkLoadDomain().
>
> Before this patch prlsdkLoadDomain() worked in 2 different cases:
> 1. It creates and initializes new domain. Then updates it from sdk
> handle.
> 2. It updates existed domain from sdk handle.
> In this patch we remove code which creates new domain from LoadDomain()
> and move it to prlsdkNewDomain().
> Now prlsdkLoadDomain() only updates domain from skd handle.
>
> In notification handler prlsdkHandleVmAddedEvent() we check
> the existence of a domain and if it doesn't exist we add new domain
> by calling
> prlsdkNewDomain() and load info from sdk handle via prlsdkLoadDomain().
> ---
> src/vz/vz_driver.c | 13 ++-
> src/vz/vz_sdk.c | 253
> +++++++++++++++++++++++++++--------------------------
> src/vz/vz_sdk.h | 9 +-
> 3 files changed, 146 insertions(+), 129 deletions(-)
>
> diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
> index 91a48b6..521efd4 100644
> --- a/src/vz/vz_driver.c
> +++ b/src/vz/vz_driver.c
> @@ -685,6 +685,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const
> char *xml, unsigned int flags)
> virDomainPtr retdom = NULL;
> virDomainDefPtr def;
> virDomainObjPtr olddom = NULL;
> + virDomainObjPtr newdom = NULL;
> unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
> virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
> @@ -700,6 +701,9 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const
> char *xml, unsigned int flags)
> olddom = virDomainObjListFindByUUID(privconn->domains, def->uuid);
> if (olddom == NULL) {
> virResetLastError();
> + newdom = prlsdkNewDomain(privconn, def->name, def->uuid);
> + if (!newdom)
> + goto cleanup;
> if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
> if (prlsdkCreateVm(conn, def))
> goto cleanup;
> @@ -713,8 +717,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const
> char *xml, unsigned int flags)
> goto cleanup;
> }
> - olddom = prlsdkAddDomain(privconn, def->uuid);
> - if (!olddom)
> + if (prlsdkLoadDomain(privconn, newdom))
> goto cleanup;
> } else {
> int state, reason;
> @@ -755,6 +758,12 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const
> char *xml, unsigned int flags)
> cleanup:
> if (olddom)
> virObjectUnlock(olddom);
> + if (newdom) {
> + if (!retdom)
> + virDomainObjListRemove(privconn->domains, newdom);
> + else
> + virObjectUnlock(newdom);
> + }
> virDomainDefFree(def);
> vzDriverUnlock(privconn);
> return retdom;
> diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
> index 6fb2a97..9d2bdab 100644
> --- a/src/vz/vz_sdk.c
> +++ b/src/vz/vz_sdk.c
> @@ -1236,58 +1236,35 @@ prlsdkConvertCpuMode(PRL_HANDLE sdkdom,
> virDomainDefPtr def)
> return -1;
> }
> -/*
> - * This function retrieves information about domain.
> - * If the domains is already in the domains list
> - * privconn->domains, then locked 'olddom' must be
> - * provided. If the domains must be added to the list,
> - * olddom must be NULL.
> - *
> - * The function return a pointer to a locked virDomainObj.
> - */
> -static virDomainObjPtr
> -prlsdkLoadDomain(vzConnPtr privconn,
> - PRL_HANDLE sdkdom,
> - virDomainObjPtr olddom)
> +int
> +prlsdkLoadDomain(vzConnPtr privconn, virDomainObjPtr dom)
> {
> - virDomainObjPtr dom = NULL;
> virDomainDefPtr def = NULL;
> - vzDomObjPtr pdom = NULL;
> VIRTUAL_MACHINE_STATE domainState;
> + char *home = NULL;
> PRL_UINT32 buflen = 0;
> PRL_RESULT pret;
> PRL_UINT32 ram;
> PRL_UINT32 envId;
> PRL_VM_AUTOSTART_OPTION autostart;
> + PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
> + vzDomObjPtr pdom = dom->privateData;
> virCheckNonNullArgGoto(privconn, error);
> - virCheckNonNullArgGoto(sdkdom, error);
> + virCheckNonNullArgGoto(dom, error);
> +
> + sdkdom = prlsdkSdkDomainLookupByUUID(privconn, dom->def->uuid);
> + if (sdkdom == PRL_INVALID_HANDLE)
> + return -1;
> if (!(def = virDomainDefNew()))
> goto error;
> - if (!olddom) {
> - if (VIR_ALLOC(pdom) < 0)
> - goto error;
> - pdom->cache.stats = PRL_INVALID_HANDLE;
> - pdom->cache.count = -1;
> - if (virCondInit(&pdom->cache.cond) < 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot
> initialize condition"));
> - goto error;
> - }
> - } else {
> - pdom = olddom->privateData;
> - }
> + def->virtType = dom->def->virtType;
> + def->id = dom->def->id;
> - if (STREQ(privconn->drivername, "vz"))
> - def->virtType = VIR_DOMAIN_VIRT_VZ;
> - else
> - def->virtType = VIR_DOMAIN_VIRT_PARALLELS;
> -
> - def->id = -1;
> -
> - if (prlsdkGetDomainIds(sdkdom, &def->name, def->uuid) < 0)
> + if (prlsdkGetDomainIds(sdkdom, &def->name, def->uuid))
> goto error;
> def->onReboot = VIR_DOMAIN_LIFECYCLE_RESTART;
> @@ -1318,29 +1295,33 @@ prlsdkLoadDomain(vzConnPtr privconn,
> pret = PrlVmCfg_GetEnvId(sdkdom, &envId);
> prlsdkCheckRetGoto(pret, error);
> - pdom->id = envId;
> buflen = 0;
> pret = PrlVmCfg_GetHomePath(sdkdom, NULL, &buflen);
> prlsdkCheckRetGoto(pret, error);
> - VIR_FREE(pdom->home);
> - if (VIR_ALLOC_N(pdom->home, buflen) < 0)
> + if (VIR_ALLOC_N(home, buflen) < 0)
> goto error;
> - pret = PrlVmCfg_GetHomePath(sdkdom, pdom->home, &buflen);
> + pret = PrlVmCfg_GetHomePath(sdkdom, home, &buflen);
> prlsdkCheckRetGoto(pret, error);
> - /* For VMs pdom->home is actually /directory/config.pvs */
> + /* For VMs home is actually /directory/config.pvs */
> if (!IS_CT(def)) {
> /* Get rid of /config.pvs in path string */
> - char *s = strrchr(pdom->home, '/');
> + char *s = strrchr(home, '/');
> if (s)
> *s = '\0';
> }
> pret = PrlVmCfg_GetAutoStart(sdkdom, &autostart);
> prlsdkCheckRetGoto(pret, error);
> + if (autostart != PAO_VM_START_ON_LOAD &&
> + autostart != PAO_VM_START_MANUAL) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unknown autostart mode: %X"), autostart);
> + goto error;
> + }
> if (prlsdkGetDomainState(sdkdom, &domainState) < 0)
> goto error;
> @@ -1363,38 +1344,6 @@ prlsdkLoadDomain(vzConnPtr privconn,
> goto error;
> }
> - if (olddom) {
> - /* assign new virDomainDef without any checks */
> - /* we can't use virDomainObjAssignDef, because it checks
> - * for state and domain name */
> - dom = olddom;
> - virDomainDefFree(dom->def);
> - dom->def = def;
> - } else {
> - if (!(dom = virDomainObjListAdd(privconn->domains, def,
> - privconn->xmlopt,
> - 0, NULL)))
> - goto error;
> - }
> - /* dom is locked here */
> -
> - dom->privateData = pdom;
> - dom->privateDataFreeFunc = prlsdkDomObjFreePrivate;
> - dom->persistent = 1;
> -
> - switch (autostart) {
> - case PAO_VM_START_ON_LOAD:
> - dom->autostart = 1;
> - break;
> - case PAO_VM_START_MANUAL:
> - dom->autostart = 0;
> - break;
> - default:
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Unknown autostart mode: %X"), autostart);
> - goto error;
> - }
> -
> if (prlsdkConvertDomainState(domainState, envId, dom) < 0)
> goto error;
> @@ -1404,24 +1353,27 @@ prlsdkLoadDomain(vzConnPtr privconn,
> pdom->sdkdom = sdkdom;
> }
> - return dom;
> - error:
> - if (dom && !olddom) {
> - /* Domain isn't persistent means that we haven't yet set
> - * prlsdkDomObjFreePrivate and should call it manually
> - */
> - if (!dom->persistent)
> - prlsdkDomObjFreePrivate(pdom);
> + /* assign new virDomainDef without any checks
> + * we can't use virDomainObjAssignDef, because it checks
> + * for state and domain name */
> + virDomainDefFree(dom->def);
> + dom->def = def;
> + pdom->id = envId;
> + VIR_FREE(pdom->home);
> + pdom->home = home;
> - virDomainObjListRemove(privconn->domains, dom);
> - }
> - /* Delete newly allocated def only if we haven't assigned it to
> domain
> - * Otherwise we will end up with domain having invalid def
> within it
> - */
> - if (!dom)
> - virDomainDefFree(def);
> + if (autostart == PAO_VM_START_ON_LOAD)
> + dom->autostart = 1;
> + else
> + dom->autostart = 0;
> - return NULL;
> + PrlHandle_Free(sdkdom);
> + return 0;
> + error:
> + PrlHandle_Free(sdkdom);
> + VIR_FREE(home);
> + virDomainDefFree(def);
> + return -1;
> }
> int
> @@ -1429,11 +1381,13 @@ prlsdkLoadDomains(vzConnPtr privconn)
> {
> PRL_HANDLE job = PRL_INVALID_HANDLE;
> PRL_HANDLE result;
> - PRL_HANDLE sdkdom;
> + PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
> PRL_UINT32 paramsCount;
> PRL_RESULT pret;
> size_t i = 0;
> virDomainObjPtr dom;
> + unsigned char uuid[VIR_UUID_BUFLEN];
> + char *name = NULL;
> job = PrlSrv_GetVmListEx(privconn->server, PVTF_VM | PVTF_CT);
> @@ -1447,61 +1401,89 @@ prlsdkLoadDomains(vzConnPtr privconn)
> pret = PrlResult_GetParamByIndex(result, i, &sdkdom);
> if (PRL_FAILED(pret)) {
> logPrlError(pret);
> - PrlHandle_Free(sdkdom);
> goto error;
> }
> - dom = prlsdkLoadDomain(privconn, sdkdom, NULL);
> - PrlHandle_Free(sdkdom);
> + if (prlsdkGetDomainIds(sdkdom, &name, uuid))
> + goto error;
> - if (!dom)
> + if (!(dom = prlsdkNewDomain(privconn, name, uuid)))
> goto error;
> - else
> - virObjectUnlock(dom);
> +
> + if (prlsdkLoadDomain(privconn, dom)) {
> + virDomainObjListRemove(privconn->domains, dom);
> + goto error;
> + }
> +
> + VIR_FREE(name);
> + PrlHandle_Free(sdkdom);
> + sdkdom = PRL_INVALID_HANDLE;
> + virObjectUnlock(dom);
> }
> PrlHandle_Free(result);
> return 0;
> error:
> + VIR_FREE(name);
> + PrlHandle_Free(sdkdom);
> PrlHandle_Free(result);
> return -1;
> }
> -virDomainObjPtr
> -prlsdkAddDomain(vzConnPtr privconn, const unsigned char *uuid)
> -{
> - PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
> - virDomainObjPtr dom;
> -
> - dom = virDomainObjListFindByUUID(privconn->domains, uuid);
> - if (dom) {
> - /* domain is already in the list */
> - return dom;
> - }
> -
> - sdkdom = prlsdkSdkDomainLookupByUUID(privconn, uuid);
> - if (sdkdom == PRL_INVALID_HANDLE)
> - return NULL;
> -
> - dom = prlsdkLoadDomain(privconn, sdkdom, NULL);
> - PrlHandle_Free(sdkdom);
> - return dom;
> -}
> -
> int
> prlsdkUpdateDomain(vzConnPtr privconn, virDomainObjPtr dom)
> {
> PRL_HANDLE job;
> - virDomainObjPtr retdom = NULL;
> vzDomObjPtr pdom = dom->privateData;
> job = PrlVm_RefreshConfig(pdom->sdkdom);
> if (waitJob(job))
> return -1;
> - retdom = prlsdkLoadDomain(privconn, pdom->sdkdom, dom);
> - return retdom ? 0 : -1;
> + return prlsdkLoadDomain(privconn, dom);
> +}
> +
> +
> +virDomainObjPtr
> +prlsdkNewDomain(vzConnPtr privconn, char *name, const unsigned char
> *uuid)
> +{
Since this function doesn't communicate with prlsdk its name prefix
looks strange to me.
Also I would think about this function to vz_driver.c, but I don't
insist on it.
changed to vzNewDomain() and moved to vz_utils.c
> + virDomainDefPtr def = NULL;
> + virDomainObjPtr dom = NULL;
> + vzDomObjPtr pdom = NULL;
> +
> + if (!(def = virDomainDefNewFull(name, uuid, -1)))
> + goto error;
> +
> + if (VIR_ALLOC(pdom) < 0)
> + goto error;
> +
> + pdom->cache.stats = PRL_INVALID_HANDLE;
> + pdom->cache.count = -1;
> + if (virCondInit(&pdom->cache.cond) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot
> initialize condition"));
> + goto error;
> + }
> +
> + if (STREQ(privconn->drivername, "vz"))
> + def->virtType = VIR_DOMAIN_VIRT_VZ;
> + else
> + def->virtType = VIR_DOMAIN_VIRT_PARALLELS;
> +
> + if (!(dom = virDomainObjListAdd(privconn->domains, def,
> + privconn->xmlopt,
> + 0, NULL)))
> + goto error;
> +
> + dom->privateData = pdom;
> + dom->privateDataFreeFunc = prlsdkDomObjFreePrivate;
> + dom->persistent = 1;
> + return dom;
> +
> + error:
> + virDomainDefFree(def);
> + VIR_FREE(pdom);
> + return NULL;
> }
> static int prlsdkSendEvent(vzConnPtr privconn,
> @@ -1618,15 +1600,36 @@ prlsdkHandleVmAddedEvent(vzConnPtr privconn,
> unsigned char *uuid)
> {
> virDomainObjPtr dom = NULL;
> + PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
> + char *name = NULL;
> - dom = prlsdkAddDomain(privconn, uuid);
> - if (dom == NULL)
> - return;
> + dom = virDomainObjListFindByUUID(privconn->domains, uuid);
> + if (!dom) {
> + sdkdom = prlsdkSdkDomainLookupByUUID(privconn, uuid);
> + if (sdkdom == PRL_INVALID_HANDLE)
> + goto cleanup;
> +
> + if (prlsdkGetDomainIds(sdkdom, &name, NULL))
> + goto cleanup;
> +
> + if (!(dom = prlsdkNewDomain(privconn, name, uuid)))
> + goto cleanup;
> +
> + if (prlsdkLoadDomain(privconn, dom)) {
> + virDomainObjListRemove(privconn->domains, dom);
> + dom = NULL;
> + goto cleanup;
> + }
> + }
> prlsdkSendEvent(privconn, dom, VIR_DOMAIN_EVENT_DEFINED,
> VIR_DOMAIN_EVENT_DEFINED_ADDED);
> - virObjectUnlock(dom);
> + cleanup:
> + if (dom)
> + virObjectUnlock(dom);
> + VIR_FREE(name);
> + PrlHandle_Free(sdkdom);
> return;
> }
> diff --git a/src/vz/vz_sdk.h b/src/vz/vz_sdk.h
> index ff6be07..f2d3e35 100644
> --- a/src/vz/vz_sdk.h
> +++ b/src/vz/vz_sdk.h
> @@ -30,9 +30,14 @@ int prlsdkConnect(vzConnPtr privconn);
> void prlsdkDisconnect(vzConnPtr privconn);
> int
> prlsdkLoadDomains(vzConnPtr privconn);
> -virDomainObjPtr
> -prlsdkAddDomain(vzConnPtr privconn, const unsigned char *uuid);
> int prlsdkUpdateDomain(vzConnPtr privconn, virDomainObjPtr dom);
> +virDomainObjPtr
> +prlsdkNewDomain(vzConnPtr privconn,
> + char *name,
> + const unsigned char *uuid);
> +int
> +prlsdkLoadDomain(vzConnPtr privconn,
> + virDomainObjPtr dom);
> int prlsdkSubscribeToPCSEvents(vzConnPtr privconn);
> void prlsdkUnsubscribeFromPCSEvents(vzConnPtr privconn);
> PRL_RESULT prlsdkStart(PRL_HANDLE sdkdom);
3224
days inactive
3225
days old
15 comments
3 participants
participants (3)
-
Maxim Nestratov -
Mikhail Feoktistov -
Nikolay Shirokovskiy