Re: [libvirt] Re: [PATCH] Implement finer grained migration control for Xen

Anno domini 2009 Chris Lalancette scripsit: > > Attached you can find a patch implementing the same flags for Xen: > > * src/xen/xen_driver.c: Add support for VIR_MIGRATE_PERSIST_DEST flag > > * src/xen/xend_internal.c: Add support for VIR_MIGRATE_UNDEFINE_SOURCE flag > > I'm not totaly sure if there are better ways to handle all the error > > cases. The current solution seemed to be a same one for me. > Well, I do think that we need to return a proper error in all cases except for > the one with the long comment. I've pointed out where I think we need to add > error messages below. Otherwise, I think it looks good. > > diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c > > index 5273a11..18882c0 100644 > > --- a/src/xen/xen_driver.c > > +++ b/src/xen/xen_driver.c > > @@ -1204,9 +1204,53 @@ xenUnifiedDomainMigrateFinish (virConnectPtr dconn, > > const char *cookie ATTRIBUTE_UNUSED, > > int cookielen ATTRIBUTE_UNUSED, > > const char *uri ATTRIBUTE_UNUSED, > > - unsigned long flags ATTRIBUTE_UNUSED) > > + unsigned long flags) > > { > > - return xenUnifiedDomainLookupByName (dconn, dname); > > + virDomainPtr dom = NULL; > > + char *domain_xml = NULL; > > + virDomainPtr dom_new = NULL; > > + > > + dom = xenUnifiedDomainLookupByName (dconn, dname); > > + if (! dom) { > You are probably going to want to raise some error here, otherwise the user will > get back "unknown error", which isn't very helpful. > > + return NULL; > > + } > > + > > + if (flags & VIR_MIGRATE_PERSIST_DEST) { > > + domain_xml = xenDaemonDomainDumpXML (dom, 0, NULL); > > + if (! domain_xml) { > > + goto failure; > > + } > This seems whitespace damaged (using tabs instead of spaces), and also needs to > raise a proper error. > > + dom_new = xenDaemonDomainDefineXML (dconn, domain_xml); > > + if (! dom_new) { > > + /* Hmpf. Migration was successful, but making it persistent > > + * was not. If we report successful, then when this domain > > + * shuts down, management tools are in for a surprise. On the > > + * other hand, if we report failure, then the management tools > > + * might try to restart the domain on the source side, even > > + * though the domain is actually running on the destination. > > + * Return a NULL dom pointer, and hope that this is a rare > > + * situation and management tools are smart. > > + */ > > + goto failure; > > + } When thinking about the handling of these error cases, I think that they all are equal from a users/management tools point of view: The migration worked, but something in the process of making the new VM persistant failed. So if we raise an error here, we IMO should do in in all three cases or do it nowhere. What would be the additional win of telling the user, we failed to lookup the VM on the destination host (for whatever reason) or failed to dump the XML in contrast to not telling him the define went wrong? So the prefect solution would IMO be to raise some kind of FAILED_TO_MAKE_VM_PERSIST error which would indicate the situation clearly to the application above. Comments?