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

[Dmitry Guryanov]

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