[libvirt] [PATCH v5 1/9] pvs: add driver skeleton
Dmitry Guryanov
dguryanov at parallels.com
Tue May 1 13:03:47 UTC 2012
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 at parallels.com>
>> ---
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120501/61c1127b/attachment-0001.htm>
More information about the libvir-list
mailing list