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

Dmitry Guryanov <dguryanov@xxxxxxxxxxxxx> · Sat, 6 Sep 2014 20:12:45 +0400

On Thursday 04 September 2014 16:00:48 Michal Privoznik wrote:
> On 22.08.2014 19:48, Dmitry Guryanov wrote:
> > Add files parallels_sdk.c and parallels_sdk.h for code

> > 
> > @@ -999,8 +1022,17 @@ parallelsConnectClose(virConnectPtr conn)
> > 
> >       virObjectUnref(privconn->caps);
> >       virObjectUnref(privconn->xmlopt);
> >       virObjectUnref(privconn->domains);
> > 
> > +    prlsdkDisconnect(privconn);
> > 
> >       conn->privateData = NULL;
> > 
> > +    virMutexLock(&numConnsLock);
> > +    numConns--;
> > +
> > +    if (numConns == 0)
> > +        prlsdkDeinit();
> 
> This calls init & deinit several times, for instance, if there's just a
> single connection that disconnects eventually. Is it possible to call
> prlsdkInit() in parallelsRegister() and then call prlsdkDeinit() in
> parallelsUnregister()? I know the latter function doesn't exist yet,
> which brings up even more interesting question - is Deinit really needed
> or is it here just for the completeness' sake?
> 
> Because if it is so, we can drop the numConnsLock mutex :)
>

There is no problem with calling init and Deinit several times in one process and Deinit is really needed. There is no parallelsUnregister, and there is only virInitialize function, so I've decided to call prlsdkInit in connect/disconnect.

> > +
> > +    virMutexUnlock(&numConnsLock);
> > +
> > 
> >       parallelsDriverUnlock(privconn);
> >       virMutexDestroy(&privconn->lock);
> > 
> > @@ -2483,6 +2515,12 @@ parallelsRegister(void)
> > 
> >       VIR_FREE(prlctl_path);
> > 
> > +    if (virMutexInit(&numConnsLock) < 0) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                       _("cannot initialize mutex"));
> > +        return 0;
> > +    }
> > +
> > 
> >       if (virRegisterDriver(&parallelsDriver) < 0)
> >       
> >           return -1;
> >       
> >       if (parallelsStorageRegister())
> > 
> > diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c
> > new file mode 100644
> > index 0000000..bedf32d
> > --- /dev/null
> > +++ b/src/parallels/parallels_sdk.c
> > @@ -0,0 +1,234 @@
> > +/*
> > + * parallels_sdk.c: core driver functions for managing
> > + * Parallels Cloud Server hosts
> > + *
> > + * Copyright (C) 2014 Parallels, Inc.
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library.  If not, see
> > + * <http://www.gnu.org/licenses/>.
> > + *
> > + */
> > +
> > +#include <config.h>
> > +
> > +#include "virerror.h"
> > +#include "viralloc.h"
> > +
> > +#include "parallels_sdk.h"
> > +
> > +#define VIR_FROM_THIS VIR_FROM_PARALLELS
> > +#define JOB_INFINIT_WAIT_TIMEOUT UINT_MAX
> > +
> > +PRL_UINT32 defaultJobTimeout = JOB_INFINIT_WAIT_TIMEOUT;
> > +
> > +/*
> > + * Log error description
> > + */
> > +void logPrlErrorHelper(PRL_RESULT err, const char *filename,
> > +                       const char *funcname, size_t linenr)
> > +{
> > +    char *msg1, *msg2;
> 
> These two ^^ will have random value once the control enters the function...
> 
> > +    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;
> 
> And imagine, VIR_ALLOC() fails ...
> 
> > +
> > +    /* 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);
> > +
> > + out:
> > +    VIR_FREE(msg1);
> > +    VIR_FREE(msg2);
> 
> So here we free a random pointers. Ouch.
> 

Yes, it's a problem, msg1 will be NULL, because VIR_ALLOC_N set it to NULL in case of error, but msg2 will be random.

> > +}
> > +
> > +#define logPrlError(code)                          \
> > +    logPrlErrorHelper(code, __FILE__,              \
> > +                         __FUNCTION__, __LINE__)
> > +
> > +PRL_RESULT logPrlEventErrorHelper(PRL_HANDLE event, const char *filename,
> > +                                  const char *funcname, size_t linenr)
> > +{
> > +    PRL_RESULT ret, retCode;
> > +    char *msg1, *msg2;
> 
> And yet again
> 
> > +    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);
> > +    err = 0;
> > +
> > + out:
> > +    VIR_FREE(msg1);
> > +    VIR_FREE(msg2);
> > +
> > +    return err;
> > +}
> > +
> > +#define logPrlEventError(event)                    \
> > +    logPrlEventErrorHelper(event, __FILE__,        \
> > +                         __FUNCTION__, __LINE__)
> > +
> > +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 (timeout == JOB_INFINIT_WAIT_TIMEOUT)
> > +        timeout = defaultJobTimeout;
> 
> What's this good for? defaultJobTimeout = JOB_INFINITY_WAIT_TIMEOUT.
> Moreover, why does defaultJobTimeout need to be global (!) variable anyway?
> 

I'll move job timeout variable to parallelsConn structure, so at some time we can add config parameter for it.

> > +
> > +    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:
> > +    PrlHandle_Free(job);
> > +    return result;
> > +}
> > +
> > +#define getJobResult(job, timeout)                  \
> > +    getJobResultHelper(job, timeout, __FILE__,      \
> > +                         __FUNCTION__, __LINE__)
> > +
> > +int waitJobHelper(PRL_HANDLE job, unsigned int timeout,
> > +                  const char *filename, const char *funcname,
> > +                  size_t linenr)
> > +{
> > +    PRL_HANDLE result = NULL;
> > +
> > +    result = getJobResultHelper(job, timeout, filename, funcname,
> > linenr);
> > +    if (result)
> > +        PrlHandle_Free(result);
> > +
> > +    return result ? 0 : -1;
> > +}
> > +
> > +#define waitJob(job, timeout)                  \
> > +    waitJobHelper(job, timeout, __FILE__,      \
> > +                         __FUNCTION__, __LINE__)
> > +
> > +int prlsdkInit(void)
> > +{
> > +    PRL_RESULT ret;
> > +
> > +    ret = PrlApi_InitEx(PARALLELS_API_VER, PAM_SERVER, 0, 0);
> > +    if (PRL_FAILED(ret)) {
> > +        logPrlError(ret);
> > +        return -1;
> > +    }
> > +
> > +    return 0;
> > +};
> > +
> > +void prlsdkDeinit(void)
> > +{
> > +    PrlApi_Deinit();
> > +};
> > +
> > +int prlsdkConnect(parallelsConnPtr privconn)
> > +{
> > +    PRL_RESULT ret;
> > +    PRL_HANDLE job = PRL_INVALID_HANDLE;
> > +
> > +    ret = PrlSrv_Create(&privconn->server);
> > +    if (PRL_FAILED(ret)) {
> > +        logPrlError(ret);
> > +        return -1;
> > +    }
> > +
> > +    job = PrlSrv_LoginLocalEx(privconn->server, NULL, 0,
> > +                              PSL_HIGH_SECURITY,
> > PACF_NON_INTERACTIVE_MODE); +
> > +    if (waitJob(job, JOB_INFINIT_WAIT_TIMEOUT)) {
> > +        PrlHandle_Free(privconn->server);
> > +        return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +void prlsdkDisconnect(parallelsConnPtr privconn)
> > +{
> > +    PRL_HANDLE job;
> > +
> > +    job = PrlSrv_Logoff(privconn->server);
> > +    waitJob(job, JOB_INFINIT_WAIT_TIMEOUT);
> > +
> > +    PrlHandle_Free(privconn->server);
> > +}
> > diff --git a/src/parallels/parallels_sdk.h b/src/parallels/parallels_sdk.h
> > new file mode 100644
> > index 0000000..e4710ec
> > --- /dev/null
> > +++ b/src/parallels/parallels_sdk.h
> > @@ -0,0 +1,30 @@
> > +/*
> > + * parallels_sdk.h: core driver functions for managing
> > + * Parallels Cloud Server hosts
> > + *
> > + * Copyright (C) 2014 Parallels, Inc.
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library.  If not, see
> > + * <http://www.gnu.org/licenses/>.
> > + *
> > + */
> > +
> > +#include <Parallels.h>
> > +
> > +#include "parallels_utils.h"
> > +
> > +int prlsdkInit(void);
> > +void prlsdkDeinit(void);
> > +int prlsdkConnect(parallelsConnPtr privconn);
> > +void prlsdkDisconnect(parallelsConnPtr privconn);
> 
> So you're exposing only 4 functions, but in .c much more functions is
> introduced. Shouldn't they be made static?

OK, I agree, those functions should be static.

> 
> > diff --git a/src/parallels/parallels_utils.h
> > b/src/parallels/parallels_utils.h index 599e2c5..095c104 100644
> > --- a/src/parallels/parallels_utils.h
> > +++ b/src/parallels/parallels_utils.h
> > @@ -23,6 +23,8 @@
> > 
> >   #ifndef PARALLELS_UTILS_H
> >   # define PARALLELS_UTILS_H
> > 
> > +# include <Parallels.h>
> > +
> > 
> >   # include "driver.h"
> >   # include "conf/domain_conf.h"
> >   # include "conf/storage_conf.h"
> > 
> > @@ -40,6 +42,7 @@
> > 
> >   struct _parallelsConn {
> >   
> >       virMutex lock;
> >       virDomainObjListPtr domains;
> > 
> > +    PRL_HANDLE server;
> > 
> >       virStoragePoolObjList pools;
> >       virNetworkObjList networks;
> >       virCapsPtr caps;
> 
> Otherwise I find this patch okay.
> 
> Michal

-- 
Dmitry Guryanov
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list