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

Re: [libvirt] [PATCH v8 1/8] parallels: add driver skeleton



On 07/10/2012 06:53 PM, Peter Krempa wrote:
On 07/05/12 12:58, Dmitry Guryanov wrote:

Parallels Virtuozzo Server is a cloud-ready virtualization
solution that allows users to simultaneously run multiple virtual
machines and containers on the same physical server.

Current name of this product is Parallels Server Bare Metal and
....
+static int
+parallelsGetVersion(virConnectPtr conn ATTRIBUTE_UNUSED, unsigned long *hvVer)
+{
+    /* TODO */
+    *hvVer = 6;

I hope this hack gets updated in a patch later on. Also this produces following output:

virsh # version
Compiled against library: libvir 0.9.13
Using library: libvir 0.9.13
Using API: PARALLELS 0.9.13
Running hypervisor: PARALLELS 0.0.6


Yes, I'll fix this soon.


+    return 0;
+}
+
+static virDriver parallelsDriver = {
+    .no = VIR_DRV_PARALLELS,
+    .name = "PARALLELS",
+    .open = parallelsOpen,            /* 0.10.0 */
+    .close = parallelsClose,          /* 0.10.0 */
+    .version = parallelsGetVersion,   /* 0.10.0 */
+    .getHostname = virGetHostname,      /* 0.10.0 */
+    .nodeGetInfo = nodeGetInfo,      /* 0.10.0 */
+    .getCapabilities = parallelsGetCapabilities,      /* 0.10.0 */
+};
+
+/**
+ * parallelsRegister:
+ *
+ * Registers the parallels driver
+ */
+int
+parallelsRegister(void)
+{
+    char *prlctl_path;
+
+    prlctl_path = virFindFileInPath(PRLCTL);
+    if (!prlctl_path) {
+        parallelsError(VIR_ERR_INTERNAL_ERROR, "%s",
+                 _("Can't find prlctl command in the PATH env"));
+        return VIR_DRV_OPEN_ERROR;
+    }

Memory leak: virFindFileInPath states:
/*
 * Finds a requested executable file in the PATH env. e.g.:
 * "kvm-img" will return "/usr/bin/kvm-img"
 *
 * You must free the result
 */
char *virFindFileInPath(const char *file)

VIR_FREE(prctl_path)

Also this piece of code is somewhat user-unfriendly. If you don't have the prlctl command, the driver is not registered and for some reason the error message is not present in the logs.

Eric Blake suggested moving this check from parallelsOpen to parallelsRegister, http://www.redhat.com/archives/libvir-list/2012-May/msg00026.html.


+
+    if (virRegisterDriver(&parallelsDriver) < 0)
+        return -1;
+
+    return 0;
+}
diff --git a/src/parallels/parallels_driver.h b/src/parallels/parallels_driver.h
new file mode 100644
index 0000000..c04db35
--- /dev/null
+++ b/src/parallels/parallels_driver.h
@@ -0,0 +1,51 @@
+/*
+ * parallels_driver.c: core driver functions for managing
+ * Parallels Virtuozzo Server hosts
+ *
+ * Copyright (C) 2012 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, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ */
+
+#ifndef PARALLELS_DRIVER_H
+# define PARALLELS_DRIVER_H
+
+
+# include "domain_conf.h"
+# include "storage_conf.h"
+# include "domain_event.h"
+
+# define parallelsError(code, ...) \
+        virReportErrorHelper(VIR_FROM_TEST, code, __FILE__,         \

s/VIR_FROM_TEST/VIR_FROM_THIS/

+ __FUNCTION__, __LINE__, __VA_ARGS__)
+# define PRLCTL      "prlctl"
+
+
+struct _parallelsConn {
+    virMutex lock;
+    virDomainObjList domains;
+    virStoragePoolObjList pools;
+    virCapsPtr caps;
+    virDomainEventStatePtr domainEventState;
+};
+
+typedef struct _parallelsConn parallelsConn;
+
+typedef struct _parallelsConn *parallelsConnPtr;

All of the above definitions belong to the driver .c file as we don't want to expose the internals.

+
+int parallelsRegister(void);
+
+#endif
diff --git a/src/util/virterror.c b/src/util/virterror.c
index cb37be0..7c0119f 100644
--- a/src/util/virterror.c
+++ b/src/util/virterror.c
@@ -99,7 +99,8 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST,

                "URI Utils", /* 45 */
                "Authentication Utils",
-              "DBus Utils"
+              "DBus Utils",
+              "Parallels Cloud Server"
      )

I'm attaching a patch that contains my findings. I'm inclining to giving an ACK to this patch with the changes I suggested, but I'd be glad if somebody else could have a quick look. Let's see how the rest of the series works.

Peter


Thanks, I agree with all your comments and fixes.

--
Dmitry Guryanov


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