Re: [Libvir] Unified Xen patch (second version)
On Fri, Mar 30, 2007 at 02:27:46PM +0100, Richard W.M. Jones wrote:
Ignore the just posted "first complete version". It had
some stupid
stuff going on with connection opens. This version fixes all that, and
passes 'make check'.
/* These dispatch functions follow the model used historically
* by libvirt.c -- trying each low-level Xen driver in turn
* until one succeeds. However since we know what low-level
* drivers can perform which functions, it is probably better
* in future to optimise these dispatch functions to just call
* the single function (or small number of appropriate functions)
* in the low level drivers directly.
*/
For now, keeping the same internal driver model will help debugging any
issues in this conversion. I agree that we should later re-factor this
further to explicitly call the appropriate methods directly because I
don't think merely iterating over driver is particularly useful. For
any given libvirt method is is easy to define exactly how we should fetch
the info.
The current situation is a little odd - if we doing 'pause' then we should
always just go straight to the hypervisor. If that fails there is no point
trying any others because both XenD / XenStored ultimately call into the
hypervisor too.
I don't see the point if the XenStoreD driver at all. It is lower priority
than the XenD driver is, and since XenD driver has 100% coverage of all
APIs there is no circumstance in which the XenStoreD driver will ever be
called. There's a couple of helper methods in the xenstored.c file which
are used directly be xend.c / xml.c, but aside from that the rest is 100%
untested by any of us.
static int
xenUnifiedDomainSuspend (virDomainPtr dom)
{
int i;
/* Try non-hypervisor methods first, then hypervisor direct method
* as a last resort.
*/
for (i = 0; i < nb_drivers; ++i)
if (i != hypervisor_offset &&
drivers[i]->domainSuspend &&
drivers[i]->domainSuspend (dom) == 0)
return 0;
if (drivers[hypervisor_offset]->domainSuspend &&
drivers[hypervisor_offset]->domainSuspend (dom) == 0)
return 0;
return -1;
}
Why do we try the non-hypervisor method first ? What does XenD give us that
we don't get just by going to the HV immediately, aside from higher overheads.
Likewise for Resume/Destroy/SetVCPUs. If there is a compelling reason why
we must call XenD, then we shouldn't try to fallback to the HV at all. If
there isn't a compelling reason, then we should try HV first.
Was this how the existing code already worked, if so i guess we should leave
as is until we can cleanup like I described earlier ? If any knows why, we
should at least comment this voodoo...
+static int
+qemuNetworkClose (virConnectPtr conn ATTRIBUTE_UNUSED)
+{
+ /* NB: This will fail to close the connection properly
+ * qemuRegister is changed so that it only registers the
+ * network (not the connection driver).
+ */
+ return 0;
+}
I'm not sure what you mean by this comment ? I'm fairly sure we need to have
code in qemuNetworkClose() though to close the QEMU socket when the XenD
driver is used QEMU driver for the networking APIs.
Apart from the 2 questions about suspend/resume/destroy APIs and the QEMU
networking code, this patch looks fine for inclusion to me. Although it is
a big patch, it is basically a straight forward re-factoring with no major
operational changes aside from in the open/close routines.
I'm going to actually try it out for real shortly....
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|