[libvirt] [PATCH v2 0/2] parallels: use parallels SDK instead of prlctl tool

This patchset begins reworking of parallels driver. We have published Opensource version of parallels SDK (under LGPL license), so libvirt can link with it. Dmitry Guryanov (2): parallels: build with parallels SDK parallels: login to parallels SDK configure.ac | 24 ++-- po/POTFILES.in | 1 + src/Makefile.am | 8 +- src/parallels/parallels_driver.c | 40 ++++++- src/parallels/parallels_sdk.c | 241 +++++++++++++++++++++++++++++++++++++++ src/parallels/parallels_sdk.h | 30 +++++ src/parallels/parallels_utils.h | 4 + 7 files changed, 334 insertions(+), 14 deletions(-) create mode 100644 src/parallels/parallels_sdk.c create mode 100644 src/parallels/parallels_sdk.h -- 1.9.3

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> --- changes in v2: * run pkgconfig check only if --with-parallels set to yes or check configure.ac | 24 +++++++++++++----------- src/Makefile.am | 4 +++- 2 files changed, 16 insertions(+), 12 deletions(-) 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"; then - AC_DEFINE_UNQUOTED([WITH_PARALLELS], 1, [whether Parallels driver is enabled]) + 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 + + 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"]) diff --git a/src/Makefile.am b/src/Makefile.am index 46e411e..83618ef 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1387,7 +1387,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 -- 1.9.3

On Sat, Sep 06, 2014 at 08:28:09PM +0400, 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> ---
changes in v2: * run pkgconfig check only if --with-parallels set to yes or check
configure.ac | 24 +++++++++++++----------- src/Makefile.am | 4 +++- 2 files changed, 16 insertions(+), 12 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

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: * fix error path in logPrlErrorHelper and logPrlEventErrorHelper * move paralles SDK job timeout setting to parallelsConn * make functions, which are not used in other files, static. po/POTFILES.in | 1 + src/Makefile.am | 4 +- src/parallels/parallels_driver.c | 40 ++++++- src/parallels/parallels_sdk.c | 241 +++++++++++++++++++++++++++++++++++++++ src/parallels/parallels_sdk.h | 30 +++++ src/parallels/parallels_utils.h | 4 + 6 files changed, 318 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 83618ef..2fe5335 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..1e4b070 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(privconn)) { + 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(); + + 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..22afd61 --- /dev/null +++ b/src/parallels/parallels_sdk.c @@ -0,0 +1,241 @@ +/* + * 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 + */ +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); + + out: + 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); + err = 0; + + out: + 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: + PrlHandle_Free(job); + return result; +} + +#define getJobResult(job, timeout) \ + getJobResultHelper(job, timeout, __FILE__, \ + __FUNCTION__, __LINE__) + +static 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(parallelsConnPtr privconn) +{ + PRL_RESULT ret; + + ret = PrlApi_InitEx(PARALLELS_API_VER, PAM_SERVER, 0, 0); + if (PRL_FAILED(ret)) { + logPrlError(ret); + return -1; + } + + privconn->jobTimeout = JOB_INFINIT_WAIT_TIMEOUT; + + 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, privconn->jobTimeout)) { + PrlHandle_Free(privconn->server); + return -1; + } + + return 0; +} + +void +prlsdkDisconnect(parallelsConnPtr privconn) +{ + PRL_HANDLE job; + + job = PrlSrv_Logoff(privconn->server); + waitJob(job, privconn->jobTimeout); + + PrlHandle_Free(privconn->server); +} diff --git a/src/parallels/parallels_sdk.h b/src/parallels/parallels_sdk.h new file mode 100644 index 0000000..cefe67d --- /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(parallelsConnPtr privconn); +void prlsdkDeinit(void); +int prlsdkConnect(parallelsConnPtr privconn); +void prlsdkDisconnect(parallelsConnPtr privconn); diff --git a/src/parallels/parallels_utils.h b/src/parallels/parallels_utils.h index 599e2c5..aef590f 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,8 @@ struct _parallelsConn { virMutex lock; virDomainObjListPtr domains; + PRL_HANDLE server; + PRL_UINT32 jobTimeout; virStoragePoolObjList pools; virNetworkObjList networks; virCapsPtr caps; -- 1.9.3

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.
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.
+ + 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 -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

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

On Thu, Sep 11, 2014 at 06:19:23PM +0400, Dmitry Guryanov wrote:
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,
Ok cool. In the configure.ac pkg-config check be sure to add a version number comparison, so that we guarantee a new enough library to include the ref counting you add. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Friday 12 September 2014 10:11:52 Daniel P. Berrange wrote:
On Thu, Sep 11, 2014 at 06:19:23PM +0400, Dmitry Guryanov wrote:
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,
Ok cool.
In the configure.ac pkg-config check be sure to add a version number comparison, so that we guarantee a new enough library to include the ref counting you add.
We haven't released opensource version of parallels sdk yet, there is only git available, I've committed patch already. So the first released version will have ability to run init and deinit several times.
Regards, Daniel
-- Dmitry Guryanov
participants (2)
-
Daniel P. Berrange
-
Dmitry Guryanov