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

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

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