On Thursday 11 September 2014 12:09:20 Daniel P. Berrange wrote:
> 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.
>
I can move reference counting to parallels SDK,
> > 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.
>
OK, thanks, I'll fix it.
> > +
>
> > + 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
--
Dmitry Guryanov
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list