Re: [libvirt] [PATCH 2/3] parallels: move up updating autostart parameter in prlsdkLoadDomain
21.05.2015 9:55, Dmitry Guryanov пишет:
On 05/18/2015 05:44 PM, Maxim Nestratov wrote:
> It is better to get all necessary parameters and check them on newly
> created configuration before actually creating a domain with them or
> applying them to an existing domain.
What problem could it cause?
First of all if we ask something from parallels
dispatcher it takes time
and doing such things with locked domain isn't a good practice. I am not
saying that PrlVmCfg_GetAutoStart will go to dispatcher but other SDK
functions will. What I wanted here is to group SDK calls together.
Secondly, there can be a race when we call prlsdkLoadDomain function
from domain creation event handler after the domain is deleted by
concurrent extern call. So it's better to fail before we actually create
a domain.
>
> Signed-off-by: Maxim Nestratov <mnestratov(a)parallels.com>
> ---
> src/parallels/parallels_sdk.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/src/parallels/parallels_sdk.c
> b/src/parallels/parallels_sdk.c
> index faae1ae..5c15e94 100644
> --- a/src/parallels/parallels_sdk.c
> +++ b/src/parallels/parallels_sdk.c
> @@ -1312,6 +1312,9 @@ prlsdkLoadDomain(parallelsConnPtr privconn,
> *s = '\0';
> }
> + pret = PrlVmCfg_GetAutoStart(sdkdom, &autostart);
> + prlsdkCheckRetGoto(pret, error);
> +
> if (virDomainDefAddImplicitControllers(def) < 0)
> goto error;
> @@ -1349,15 +1352,6 @@ prlsdkLoadDomain(parallelsConnPtr privconn,
> dom->privateDataFreeFunc = prlsdkDomObjFreePrivate;
> dom->persistent = 1;
> - if (prlsdkGetDomainState(sdkdom, &domainState) < 0)
> - goto error;
> -
> - if (prlsdkConvertDomainState(domainState, envId, dom) < 0)
> - goto error;
> -
> - pret = PrlVmCfg_GetAutoStart(sdkdom, &autostart);
> - prlsdkCheckRetGoto(pret, error);
> -
> switch (autostart) {
> case PAO_VM_START_ON_LOAD:
> dom->autostart = 1;
> @@ -1371,6 +1365,12 @@ prlsdkLoadDomain(parallelsConnPtr privconn,
> goto error;
> }
> + if (prlsdkGetDomainState(sdkdom, &domainState) < 0)
> + goto error;
> +
Why don't you move this ^^ call?
Makes sence even more than PrlVmCfg_GetAutoStart. Just missed it.
I'll add it in the second version of the patchset.
> + if (prlsdkConvertDomainState(domainState, envId, dom) < 0)
> + goto error;
> +
> if (!pdom->sdkdom) {
> pret = PrlHandle_AddRef(sdkdom);
> prlsdkCheckRetGoto(pret, error);