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

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


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