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

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

 



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).

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

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

> +++ 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.

-- 
Eric Blake   eblake@xxxxxxxxxx    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]