Re: Unified Xen patch (second version)

["Richard W.M. Jones" <rjones@xxxxxxxxxx>]

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.

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature