Re: [libvirt] [PATCH 03/16] Introduce migration cookies to QEMU driver

On 05/12/2011 10:04 AM, Daniel P. Berrange wrote: > The migration protocol has support for a 'cookie' parameter which > is an opaque array of bytes as far as libvirt is concerned. Drivers > may use this for passing around arbitrary extra data they might > need during migration. The QEMU driver needs to do a few things: > > +static int > +qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig, > + xmlXPathContextPtr ctxt, > + int flags ATTRIBUTE_UNUSED) > +{ > + char uuidstr[VIR_UUID_STRING_BUFLEN]; > + char *tmp; > + > + /* We don't store the uuid, name, hostname, or hostuuid > + * values. We just compare them to local data to do some > + * sanity checking on migration operation > + */ Should we do sanity checking that no unknown XML elements appear in the cookie? And/or validate that flags contains no unexpected bits? That is, should you insert virCheckFlags(0, -1) here, or in qemuMigrationEatCookie? And rather than just using virXPathString to probe whether a particular XML element is present, shouldn't you instead do an iteration over all XML elements in order to detect unrecognized elements? Otherwise, what I'm afraid of is that the cookie eater (whether the destination eating the cookie from Begin, or the source eating the cookie from Prepare) will be running an earlier libvirt version than the baker; if the baker added a mandatory flag to the cookie, but the eater is unaware to look for that element and silently ignores it, then we risk silent botching of migration. Do we have enough infrastructure in place for source and destination to agree on what features are mandatory vs. optional in the cookie, to allow for omission of optional elements that would make migration nicer but aren't fatal if left out? That is, a baker can always try a flag, then retry without the flag, but retries can get expensive if there were an alternative to first do a capability query to determine the common subset of flags to use in the first place.

> @@ -342,6 +612,15 @@ qemuMigrationPrepareTunnel(struct qemud_driver *driver, > event = virDomainEventNewFromObj(vm, > VIR_DOMAIN_EVENT_STARTED, > VIR_DOMAIN_EVENT_STARTED_MIGRATED); > + > + if (qemuMigrationBakeCookie(mig, driver, vm, cookieout, cookieoutlen, 0) < 0) { > + /* We could tear down the whole guest here, but > + * cookie data is (so far) non-critical, so that > + * seems a little harsh. We'll just warn for now. > + */ > + VIR_WARN("Unable to encode migration cookie"); > + } > + > ret = 0; > > endjob: > @@ -369,7 +648,7 @@ cleanup: > virDomainObjUnlock(vm); > if (event) > qemuDomainEventQueue(driver, event); > - qemuDriverUnlock(driver); > + qemuMigrationCookieFree(mig); Umm, did you really intend to drop the qemuDriverUnlock line?