On 22.08.2014 19:48, Dmitry Guryanov wrote:

Executing prlctl command is not an optimal way to interact with Parallels Cloud Server (PCS), it's better to use parallels SDK, which is a remote API to paralles dispatcher service.

We prepared opensource version of this SDK and published it on github, it's distributed under LGPL license. Here is a git repo: https://github.com/Parallels/parallels-sdk.

To build with parallels SDK user should get compiler and linker options from pkg-config 'parallels-sdk' file. So fix checks in configure script and build with parallels SDK, if that pkg-config file exists and add gcc options to makefile.

Signed-off-by: Dmitry Guryanov <dguryanov@parallels.com> --- configure.ac | 11 +++++------ src/Makefile.am | 4 +++- 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/configure.ac b/configure.ac index f93c6c2..4318d24 100644 --- a/configure.ac +++ b/configure.ac @@ -1046,15 +1046,14 @@ dnl dnl Checks for the Parallels driver dnl

+PKG_CHECK_MODULES([PARALLELS_SDK], [parallels-sdk], [PARALLELS_SDK_FOUND=yes], [PARALLELS_SDK_FOUND=no]) + if test "$with_parallels" = "check"; then - with_parallels=$with_linux - if test ! $host_cpu = 'x86_64'; then - with_parallels=no - fi + with_parallels=$PARALLELS_SDK_FOUND fi

This run the check even if configure is run with --without-parallels. I'd feel better if it follows the pattern used in the rest of --with-* attributes.

-if test "$with_parallels" = "yes" && test "$with_linux" = "no"; then - AC_MSG_ERROR([The Parallels driver can be enabled on Linux only.]) +if test "$with_parallels" = "yes" && test "$PARALLELS_SDK_FOUND" = "no"; then + AC_MSG_ERROR([Parallels Virtualization SDK is needed to build the Parallels driver.]) fi

if test "$with_parallels" = "yes"; then diff --git a/src/Makefile.am b/src/Makefile.am index 538530e..dad7c7f 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1352,7 +1352,9 @@ if WITH_PARALLELS noinst_LTLIBRARIES += libvirt_driver_parallels.la libvirt_la_BUILT_LIBADD += libvirt_driver_parallels.la libvirt_driver_parallels_la_CFLAGS = \ - -I$(top_srcdir)/src/conf $(AM_CFLAGS) + -I$(top_srcdir)/src/conf $(AM_CFLAGS) \ + $(PARALLELS_SDK_CFLAGS) +libvirt_driver_parallels_la_LIBADD = $(PARALLELS_SDK_LIBS) libvirt_driver_parallels_la_SOURCES = $(PARALLELS_DRIVER_SOURCES) endif WITH_PARALLELS

This is the diff I'm looking for: diff --git a/configure.ac b/configure.ac index b4fb99a..0062d5d 100644 --- a/configure.ac +++ b/configure.ac @@ -1046,19 +1046,21 @@ dnl dnl Checks for the Parallels driver dnl -if test "$with_parallels" = "check"; then - with_parallels=$with_linux - if test ! $host_cpu = 'x86_64'; then - with_parallels=no - fi -fi -if test "$with_parallels" = "yes" && test "$with_linux" = "no"; then - AC_MSG_ERROR([The Parallels driver can be enabled on Linux only.]) -fi +if test "$with_parallels" = "yes" || + test "$with_parallels" = "check"; then + PKG_CHECK_MODULES([PARALLELS_SDK], [parallels-sdk], + [PARALLELS_SDK_FOUND=yes], [PARALLELS_SDK_FOUND=no]) + + if test "$with_parallels" = "yes" && test "$PARALLELS_SDK_FOUND" = "no"; then + AC_MSG_ERROR([Parallels Virtualization SDK is needed to build the Parallels driver.]) + fi -if test "$with_parallels" = "yes"; then - AC_DEFINE_UNQUOTED([WITH_PARALLELS], 1, [whether Parallels driver is enabled]) + with_parallels=$PARALLELS_SDK_FOUND + if test "$with_parallels" = "yes"; then + AC_DEFINE_UNQUOTED([WITH_PARALLELS], 1, + [whether Parallels driver is enabled]) + fi fi AM_CONDITIONAL([WITH_PARALLELS], [test "$with_parallels" = "yes"]) Michal

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@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