[libvirt] [PATCH v2 2/2] parallels: login to parallels SDK
Michal Privoznik
mprivozn at redhat.com
Thu Sep 4 14:00:48 UTC 2014
On 22.08.2014 19:48, 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.
>
> Signed-off-by: Dmitry Guryanov <dguryanov at parallels.com>
> ---
>
> Changes in v2:
> * remove unneded parallelsStateDriver
> * add missing parallels_sdk.c and parallels_sdk.h
>
> po/POTFILES.in | 1 +
> src/Makefile.am | 4 +-
> src/parallels/parallels_driver.c | 40 ++++++-
> src/parallels/parallels_sdk.c | 234 +++++++++++++++++++++++++++++++++++++++
> src/parallels/parallels_sdk.h | 30 +++++
> src/parallels/parallels_utils.h | 3 +
> 6 files changed, 310 insertions(+), 2 deletions(-)
> create mode 100644 src/parallels/parallels_sdk.c
> create mode 100644 src/parallels/parallels_sdk.h
>
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index f17b35f..4c1302d 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -96,6 +96,7 @@ src/openvz/openvz_driver.c
> src/openvz/openvz_util.c
> src/parallels/parallels_driver.c
> src/parallels/parallels_network.c
> +src/parallels/parallels_sdk.c
> src/parallels/parallels_utils.c
> src/parallels/parallels_utils.h
> src/parallels/parallels_storage.c
> diff --git a/src/Makefile.am b/src/Makefile.am
> index dad7c7f..d4c6465 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -787,7 +787,9 @@ PARALLELS_DRIVER_SOURCES = \
> parallels/parallels_utils.c \
> parallels/parallels_utils.h \
> parallels/parallels_storage.c \
> - parallels/parallels_network.c
> + parallels/parallels_network.c \
> + parallels/parallels_sdk.h \
> + parallels/parallels_sdk.c
>
> BHYVE_DRIVER_SOURCES = \
> bhyve/bhyve_capabilities.c \
> diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c
> index 13a7d95..2431d00 100644
> --- a/src/parallels/parallels_driver.c
> +++ b/src/parallels/parallels_driver.c
> @@ -55,6 +55,7 @@
>
> #include "parallels_driver.h"
> #include "parallels_utils.h"
> +#include "parallels_sdk.h"
>
> #define VIR_FROM_THIS VIR_FROM_PARALLELS
>
> @@ -73,6 +74,9 @@ VIR_LOG_INIT("parallels.parallels_driver");
>
> #define IS_CT(def) (STREQ_NULLABLE(def->os.type, "exe"))
>
> +unsigned int numConns = 0;
> +virMutex numConnsLock;
> +
> static int parallelsConnectClose(virConnectPtr conn);
>
> static const char * parallelsGetDiskBusName(int bus) {
> @@ -929,9 +933,25 @@ parallelsOpenDefault(virConnectPtr conn)
> if (virMutexInit(&privconn->lock) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("cannot initialize mutex"));
> - goto error;
> + goto err_free;
> + }
> +
> + virMutexLock(&numConnsLock);
> + numConns++;
> +
> + if (numConns == 1) {
> + if (prlsdkInit()) {
> + VIR_DEBUG("%s", _("Can't initialize Parallels SDK"));
> + virMutexUnlock(&numConnsLock);
> + goto err_free;
> + }
> }
>
> + virMutexUnlock(&numConnsLock);
> +
> + if (prlsdkConnect(privconn) < 0)
> + goto err_free;
> +
> if (!(privconn->caps = parallelsBuildCapabilities()))
> goto error;
>
> @@ -953,6 +973,9 @@ parallelsOpenDefault(virConnectPtr conn)
> virObjectUnref(privconn->domains);
> virObjectUnref(privconn->caps);
> virStoragePoolObjListFree(&privconn->pools);
> + prlsdkDisconnect(privconn);
> + prlsdkDeinit();
> + err_free:
> VIR_FREE(privconn);
> return VIR_DRV_OPEN_ERROR;
> }
> @@ -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 :)
> +
> + 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(¶llelsDriver) < 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.
> +}
> +
> +#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?
> +
> + 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?
> 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
More information about the libvir-list
mailing list