Re: [libvirt] [PATCH 0/9] Add basic driver for Parallels Virtuozzo Server

On Fri, Apr 13, 2012 at 10:26:14PM +0400, Dmitry Guryanov wrote: Okay, I started to review this version 2 of the driver. IIRC the first version was relying on an API which wasn't LGPL compatible so that was a no go. What I understand from that second version is that now the driver talks to the prlctl command to get information back (using JSON). That looks acceptable, but before making further review can you fix the following things I found out in reviewing quickly patch 1-3 - the driver open doesn't seems to make any check about availability of prlctl command, it should fail to open if this is not found at Open() time. - all the entry points in the driver structure are marked as 0.9.11, since 0.9.11 is out already this would need to be bumped to 0.9.12 assuming that will be the next version and the patches make it in time for that release (scheduled at the end of the month) - the configure check blindly assumes that if compiled on linux the pvs driver should be activated. Since we rely on a command line tool to provide the interface that's a relatively safe assumption, but if I understand correctly Parrallels requires a modified kernel version, which is not upstream, right ? To that extend you're at the same level as the OpenVZ driver (you rely on it underneath, right ?) Do you support all linux archs ? If not please improve the configure time check to the architecture you actually support. In general I wonder why make it a new driver instead of ramping up the existing OpenVZ one, are the 2 really incompatible, or is the existing OpenVZ not proper for current versions ? Basically I wonder if we really need 2 drivers assuming the implementation of the hypervisor is based on the same core for both, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/