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

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.

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


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