<html> <head> <meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type"> </head> <body text="#000000" bgcolor="#FFFFFF"> On 05/01/2012 03:27 AM, Eric Blake wrote: <blockquote cite="mid:4F9F1FDE.4030908@redhat.com" type="cite"> <pre wrap="">On 04/20/2012 10:01 AM, Dmitry Guryanov wrote: </pre> <blockquote type="cite"> <pre wrap="">Add driver, which can report node info only. </pre> </blockquote> <pre wrap=""> 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: </pre> <blockquote type="cite"> <blockquote type="cite"> <pre wrap="">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 - <a class="moz-txt-link-freetext" href="http://www.parallels.com/products/server/baremetal/sp/">http://www.parallels.com/products/server/baremetal/sp/</a>. This driver will work with PVS version 6.0 , beta version scheduled at 2012 Q2. </pre> </blockquote> </blockquote> <pre wrap=""> 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). </pre> </blockquote> 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 ? <blockquote cite="mid:4F9F1FDE.4030908@redhat.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap=""> 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 </pre> </blockquote> <pre wrap=""> Normally, it helps to list patch version history... </pre> <blockquote type="cite"> <pre wrap=""> Signed-off-by: Dmitry Guryanov <a class="moz-txt-link-rfc2396E" href="mailto:dguryanov@parallels.com"><dguryanov@parallels.com></a> --- </pre> </blockquote> <pre wrap=""> 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). </pre> <blockquote type="cite"> <pre wrap=""> 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 </pre> </blockquote> <pre wrap=""> 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 </pre> <blockquote type="cite"> <pre wrap=""> 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 </pre> </blockquote> <pre wrap=""> 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. </pre> </blockquote> There is a variable $<code>build_cpu</code>, we can check it instead of running uname command. <blockquote cite="mid:4F9F1FDE.4030908@redhat.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">@@ -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]) </pre> </blockquote> <pre wrap=""> Line up the ':'. </pre> <blockquote type="cite"> <pre wrap="">+++ 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 </pre> </blockquote> <pre wrap=""> 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). </pre> <blockquote type="cite"> <pre wrap="">+ +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; </pre> </blockquote> <pre wrap=""> Should we be checking for whether the PRLCTL binary even exists, before registering this driver? </pre> </blockquote> I check for prlctl binary existence in pvsOpen function, but can move the check to pvsRegister. <blockquote cite="mid:4F9F1FDE.4030908@redhat.com" type="cite"> <blockquote type="cite"> <pre wrap="">+++ 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; </pre> </blockquote> <pre wrap=""> Unusual ordering; typically, it's nice to keep case statements in the same order as the enum declarations. </pre> </blockquote> <pre class="moz-signature" cols="72">-- Dmitry Guryanov</pre> </body> </html>