Re: [libvirt] [PATCH v2 2/2] parallels: login to parallels SDK

On Sat, Sep 06, 2014 at 08:28:10PM +0400, Dmitry Guryanov wrote: > Add files parallels_sdk.c and parallels_sdk.h for code > which works with SDK, so libvirt's code will not mix with > dealing with parallels SDK. > > To use Parallels SDK you must first call PrlApi_InitEx function, > and then you will be able to connect to a server with > PrlSrv_LoginLocalEx function. When you've done you must call > PrlApi_Deinit. So let's call PrlApi_InitEx on first .connectOpen, > count number of connections and deinitialize, when this counter > becomes zero. As a general rule, even if we count the open/close calls it isn't safe to run deinit functions in libvirt. eg consider if libvirt is linked against another program or library that also uses the paralllels SDK. Libvirt does not know if the other code has stopped using the SDK. So either the reference counting must be done in PrlApi_{InitEx,Deinit} functions directly, or libvirt should simply not call PrlApi_Deinit at all. I'd probably just go with the latter, as I doubt there is any real harm to skipping deinit.

> diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c > new file mode 100644 > index 0000000..22afd61 > --- /dev/null > +++ b/src/parallels/parallels_sdk.c > @@ -0,0 +1,241 @@ > > + > +#define VIR_FROM_THIS VIR_FROM_PARALLELS The use of this constant here will mean any error message printed by libvirt will have a 'Parallels Cloud Server:' prefix on it > +static void > +logPrlErrorHelper(PRL_RESULT err, const char *filename, > + const char *funcname, size_t linenr) > +{ > + char *msg1 = NULL, *msg2 = NULL; > + PRL_UINT32 len = 0; > + > + /* Get required buffer length */ > + PrlApi_GetResultDescription(err, PRL_TRUE, PRL_FALSE, NULL, &len); > + > + if (VIR_ALLOC_N(msg1, len) < 0) > + goto out; > + > + /* get short error description */ > + PrlApi_GetResultDescription(err, PRL_TRUE, PRL_FALSE, msg1, &len); > + > + PrlApi_GetResultDescription(err, PRL_FALSE, PRL_FALSE, NULL, &len); > + > + if (VIR_ALLOC_N(msg2, len) < 0) > + goto out; > + > + /* get long error description */ > + PrlApi_GetResultDescription(err, PRL_FALSE, PRL_FALSE, msg2, &len); > + > + virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_INTERNAL_ERROR, > + filename, funcname, linenr, > + _("Parallels SDK: %s %s"), msg1, msg2); So adding 'Parallels SDK' is probably overkill here. I'd suggest just us '%s %s' instead.

> + > + out: nit-pick, we tend to use 'cleanup' as a standard label name > + VIR_FREE(msg1); > + VIR_FREE(msg2); > +} > + > +#define logPrlError(code) \ > + logPrlErrorHelper(code, __FILE__, \ > + __FUNCTION__, __LINE__) > + > +static PRL_RESULT > +logPrlEventErrorHelper(PRL_HANDLE event, const char *filename, > + const char *funcname, size_t linenr) > +{ > + PRL_RESULT ret, retCode; > + char *msg1 = NULL, *msg2 = NULL; > + PRL_UINT32 len = 0; > + int err = -1; > + > + if ((ret = PrlEvent_GetErrCode(event, &retCode))) { > + logPrlError(ret); > + return ret; > + } > + > + PrlEvent_GetErrString(event, PRL_TRUE, PRL_FALSE, NULL, &len); > + > + if (VIR_ALLOC_N(msg1, len) < 0) > + goto out; > + > + PrlEvent_GetErrString(event, PRL_TRUE, PRL_FALSE, msg1, &len); > + > + PrlEvent_GetErrString(event, PRL_FALSE, PRL_FALSE, NULL, &len); > + > + if (VIR_ALLOC_N(msg2, len) < 0) > + goto out; > + > + PrlEvent_GetErrString(event, PRL_FALSE, PRL_FALSE, msg2, &len); > + > + virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_INTERNAL_ERROR, > + filename, funcname, linenr, > + _("Parallels SDK: %s %s"), msg1, msg2); Same note here. > + err = 0; > + > + out: And here. > + VIR_FREE(msg1); > + VIR_FREE(msg2); > + > + return err; > +} > + > +#define logPrlEventError(event) \ > + logPrlEventErrorHelper(event, __FILE__, \ > + __FUNCTION__, __LINE__) > + > +static PRL_HANDLE > +getJobResultHelper(PRL_HANDLE job, unsigned int timeout, > + const char *filename, const char *funcname, > + size_t linenr) > +{ > + PRL_RESULT ret, retCode; > + PRL_HANDLE result = NULL; > + > + if ((ret = PrlJob_Wait(job, timeout))) { > + logPrlErrorHelper(ret, filename, funcname, linenr); > + goto out; > + } > + > + if ((ret = PrlJob_GetRetCode(job, &retCode))) { > + logPrlErrorHelper(ret, filename, funcname, linenr); > + goto out; > + } > + > + if (retCode) { > + PRL_HANDLE err_handle; > + > + /* Sometimes it's possible to get additional error info. */ > + if ((ret = PrlJob_GetError(job, &err_handle))) { > + logPrlErrorHelper(ret, filename, funcname, linenr); > + goto out; > + } > + > + if (logPrlEventErrorHelper(err_handle, filename, funcname, > linenr)) + logPrlErrorHelper(retCode, filename, funcname, > linenr); + > + PrlHandle_Free(err_handle); > + } else { > + ret = PrlJob_GetResult(job, &result); > + if (PRL_FAILED(ret)) { > + logPrlErrorHelper(ret, filename, funcname, linenr); > + PrlHandle_Free(result); > + result = NULL; > + goto out; > + } > + } > + > + out: Here > + PrlHandle_Free(job); > + return result; > +} > + Regards, Daniel