[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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(&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.

+}
+
+#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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]