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