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

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 ?

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

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