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