Re: [Libvir] Unified Xen patch (second version)
Daniel P. Berrange wrote:
> 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.
Ah, so don't confuse my code/comment up there with the application of
rational thought :-) I've just copied what is in libvirt.c right now.
To whit:
int
virDomainSuspend(virDomainPtr domain)
{
//....
/*
* Go though the driver registered entry points but use the
* XEN_HYPERVISOR directly only as a last mechanism
*/
for (i = 0;i < conn->nb_drivers;i++) {
if ((conn->drivers[i] != NULL) &&
(conn->drivers[i]->no != VIR_DRV_XEN_HYPERVISOR) &&
(conn->drivers[i]->domainSuspend != NULL)) {
if (conn->drivers[i]->domainSuspend(domain) == 0)
return (0);
}
}
for (i = 0;i < conn->nb_drivers;i++) {
if ((conn->drivers[i] != NULL) &&
(conn->drivers[i]->no == VIR_DRV_XEN_HYPERVISOR) &&
(conn->drivers[i]->domainSuspend != NULL)) {
if (conn->drivers[i]->domainSuspend(domain) == 0)
return (0);
}
}
//....
}
The new code in xen_unified just duplicates that functionality.
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.
So the current code is complicated and somewhat devious. In the current
code, the qemu network close just reuses qemuClose. In the unified case
this fails because it tries to double-free a structure.
My first time around this I got around the double-free by keeping track
with a usage counter. However that had its own problems, so seeing that
currently we always register the qemu connection and qemu network
drivers together, I just created a separate qemuNetworkClose function
which does nothing. If on the other hand in future we will use qemu
network without qemu connections, then we'll need to change this (hence
added comments). Note that this applies to registration (ie.
vir*Register), not whether or not we manage to connect to libvirtd.
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....
Yeah, I'm also testing it now.
Rich.
Attachments:
- smime.p7s (application/x-pkcs7-signature — 3.2 KB)