[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 05/01/2012 06:29 PM, Eric Blake wrote:
On 05/01/2012 07:03 AM, Dmitry Guryanov wrote:
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 ?
I'll churn through some more reviews in the next couple hours, if you'd
like to wait a bit more.

Thanks, I'll wait for your comments.


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.
Not $build_cpu (the machine doing the build), but $host (the machine
where the build will run).  And indeed, I see $host is
x86_64-unknown-linux-gnu for me, so the first part of the triple will
hopefully be good enough to tell 32-bit vs. 64-bit.


Is PVS really 64-bit only?
Yes, we don't build it on the other platforms and seems will not in
the future.


+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.
I guess the difference is that on systems without pvs support, checking
in open means pvs:// will be a recognized URI that always fails, while
checking in register will make pvs:// a rejected URI up front.  I don't
know which way is more user in line with the rest of libvirt, but I
personally like knowing as soon as possible that I've got issues
(getting a connection and waiting until a failed open is not as soon as
finding out that I can't even get a connection).

OK, let's check it in pvsRegister.

--
Dmitry Guryanov


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]