On Mon, May 16, 2011 at 02:05:13PM +0100, Daniel P. Berrange wrote:
> On Thu, May 12, 2011 at 01:17:21PM -0600, Eric Blake wrote:
> > 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.
>
> Well, the migration cookies were intended to be optional data,
> which if omitted, don't result in bad stuff.
>
> eg, if the graphics relocation data doesn't exist, the end user
> will simply need to manually reconnect.
>
> I think we do need some stricter checking on the lock state
> data though, to validate that the same lock manager is
> present on both sides.
>
> > > @@ -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?
>
> No, that's a merge rebase error
Actually no it wasn't. This is a bug that's being fixed. The caller of
this method, already unlocks the driver on completion, so we had a
double unlock.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list