Re: [PATCH v5 1/9] pvs: add driver skeleton

[Dmitry Guryanov <dguryanov@xxxxxxxxxxxxx>]

On 05/01/2012 03:27 AM, Eric Blake wrote:

On 04/20/2012 10:01 AM, Dmitry Guryanov wrote:

Add driver, which can report node info only.

Since this is the first commit in the series, can you please add more
information about pvs?  This content from your 0/9 message would be
useful here:

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
more information about it can be found here -
http://www.parallels.com/products/server/baremetal/sp/.

This driver will work with PVS version 6.0 , beta version
scheduled at 2012 Q2.

I'm still thinking this series might be worth including in 0.9.12, but
I'm worried about getting good reviews (as you can see, I'm only on 1/9
and ran out of time today).

Thanks for your review, Eric !

Should I correct the issues right now and resend patches, or
it's better to wait for other patches review ?

changes:
    * add me to AUTHORS
    * fix indent in preprocessor directives in pvs_driver.h
    * remove unneded include
    * remove pvs_driver.c from po/POTFILES.in

Normally, it helps to list patch version history...

Signed-off-by: Dmitry Guryanov <dguryanov@xxxxxxxxxxxxx>
---

after this line, so that it doesn't become a permanent part of
libvirt.git (that is, it is useful for review purposes, but once the
final patch is accepted, we no longer care how we got to the final patch).

 AUTHORS                     |    1 +
 cfg.mk                      |    1 +
 configure.ac                |   23 ++++
 docs/drvpvs.html.in         |   28 +++++
 include/libvirt/virterror.h |    1 +
 libvirt.spec.in             |    7 +
 mingw32-libvirt.spec.in     |    6 +
 src/Makefile.am             |   21 ++++
 src/conf/domain_conf.c      |    3 +-
 src/conf/domain_conf.h      |    1 +
 src/driver.h                |    1 +
 src/libvirt.c               |   12 ++
 src/pvs/pvs_driver.c        |  271 +++++++++++++++++++++++++++++++++++++++++++
 src/pvs/pvs_driver.h        |   51 ++++++++
 src/util/virterror.c        |    3 +
 15 files changed, 429 insertions(+), 1 deletions(-)
 create mode 100644 docs/drvpvs.html.in
 create mode 100644 src/pvs/pvs_driver.c
 create mode 100644 src/pvs/pvs_driver.h

Run 'make syntax-check' on your source; you missed at least one change:

po_check
--- ./po/POTFILES.in
+++ ./po/POTFILES.in
@@ -61,6 +61,7 @@
 src/openvz/openvz_conf.c
 src/openvz/openvz_driver.c
 src/phyp/phyp_driver.c
+src/pvs/pvs_driver.c
 src/qemu/qemu_agent.c
 src/qemu/qemu_bridge_filter.c
 src/qemu/qemu_capabilities.c
maint.mk: you have changed the set of files with translatable diagnostics;
 apply the above patch
make: *** [sc_po_check] Error 1

 dnl
+dnl Checks for the PVS driver
+dnl
+
+if test "$with_pvs" = "check"; then
+    with_pvs=$with_linux
+    if test ! $(uname -i) = 'x86_64'; then

This will not work when cross-compiling.  For that matter, 'uname -i' is
not portable.  I'm not sure what better solution there is for deciding
whether to default things to true or false based on whether the $host
will be 64-bit, but it's certainly not right to be probing uname of $build.

There is a variable $build_cpu, we can check it instead of
running uname command.

@@ -2706,6 +2728,7 @@ AC_MSG_NOTICE([     LXC: $with_lxc])
 AC_MSG_NOTICE([    PHYP: $with_phyp])
 AC_MSG_NOTICE([     ESX: $with_esx])
 AC_MSG_NOTICE([ Hyper-V: $with_hyperv])
+AC_MSG_NOTICE([  PVS: $with_pvs])

Line up the ':'.

+++ b/src/pvs/pvs_driver.c
@@ -0,0 +1,271 @@
+/*
+ * pvs_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

Not a problem with your patch, per se, but we really should be using the
FSF-preferred form of LGPLv2+ boilerplate that uses a URL rather than a
street address (that's a global cleanup to all of libvirt).

+
+static virDriver pvsDriver = {
+    .no = VIR_DRV_PVS,
+    .name = "PVS",
+    .open = pvsOpen,            /* 0.9.12 */
+    .close = pvsClose,          /* 0.9.12 */
+    .version = pvsGetVersion,   /* 0.9.12 */
+    .getHostname = virGetHostname,      /* 0.9.12 */
+    .nodeGetInfo = nodeGetInfo,      /* 0.9.12 */
+    .getCapabilities = pvsGetCapabilities,      /* 0.9.12 */
+};
+
+/**
+ * pvsRegister:
+ *
+ * Registers the pvs driver
+ */
+int
+pvsRegister(void)
+{
+    if (virRegisterDriver(&pvsDriver) < 0)
+        return -1;

Should we be checking for whether the PRLCTL binary even exists, before
registering this driver?

I check for prlctl binary existence in pvsOpen function, but
can move the check to pvsRegister.

+++ b/src/util/virterror.c
@@ -175,6 +175,9 @@ static const char *virErrorDomainName(virErrorDomain domain) {
         case VIR_FROM_HYPERV:
             dom = "Hyper-V ";
             break;
+        case VIR_FROM_PVS:
+            dom = "Parallels Virtuozzo Server ";
+            break;
         case VIR_FROM_CAPABILITIES:
             dom = "Capabilities ";
             break;

Unusual ordering; typically, it's nice to keep case statements in the
same order as the enum declarations.

-- 
Dmitry Guryanov

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list