[libvirt] [PATCH v3] Fix up basic migration.
Basic live migration was broken by the commit that added non-shared block support in two ways: 1) It added a virCheckFlags() to doNativeMigrate(). Besides the fact that typical usage of virCheckFlags() is in driver entry points, and doNativeMigrate() is not an entry point, it was missing important flags like VIR_MIGRATE_LIVE. Move the virCheckFlags to the top-level qemuDomainMigratePrepare2 and friends. 2) It also added a memory leak in qemuMonitorTextMigrate() by not freeing the memory used by virBufferContentAndReset(). This is fixed by storing the pointer in a temporary variable and freeing it at the end. With this patch in place, normal live migration works again. v3: Instead of the churn for virCheckFlagsUI and UL, instead always promote flags to an unsigned long and always use %lx for the fprintf. v2: Add back flags check, which required adding virCheckFlagsUI and virCheckFlagsUL Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/internal.h | 7 ++++--- src/qemu/qemu_driver.c | 30 +++++++++++++++++++++++++++--- src/qemu/qemu_monitor_text.c | 10 +++++++--- 3 files changed, 38 insertions(+), 9 deletions(-) diff --git a/src/internal.h b/src/internal.h index c3c5311..fab3e11 100644 --- a/src/internal.h +++ b/src/internal.h @@ -212,15 +212,16 @@ */ # define virCheckFlags(supported, retval) \ do { \ - if ((flags & ~(supported))) { \ + unsigned long __unsuppflags = flags & ~(supported); \ + if (__unsuppflags) { \ virReportErrorHelper(NULL, \ VIR_FROM_THIS, \ VIR_ERR_INVALID_ARG, \ __FILE__, \ __FUNCTION__, \ __LINE__, \ - _("%s: unsupported flags (0x%x)"), \ - __FUNCTION__, flags & ~(supported)); \ + _("%s: unsupported flags (0x%lx)"), \ + __FUNCTION__, __unsuppflags); \ return retval; \ } \ } while (0) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 77e71cc..941b482 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10175,6 +10175,15 @@ qemudDomainMigratePrepare2 (virConnectPtr dconn, int ret = -1; int internalret; + virCheckFlags(VIR_MIGRATE_LIVE | + VIR_MIGRATE_PEER2PEER | + VIR_MIGRATE_TUNNELLED | + VIR_MIGRATE_PERSIST_DEST | + VIR_MIGRATE_UNDEFINE_SOURCE | + VIR_MIGRATE_PAUSED | + VIR_MIGRATE_NON_SHARED_DISK | + VIR_MIGRATE_NON_SHARED_INC, -1); + *uri_out = NULL; qemuDriverLock(driver); @@ -10356,9 +10365,6 @@ static int doNativeMigrate(struct qemud_driver *driver, qemuDomainObjPrivatePtr priv = vm->privateData; unsigned int background_flags = 0; - virCheckFlags(VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC, - -1); - /* Issue the migrate command. */ if (STRPREFIX(uri, "tcp:") && !STRPREFIX(uri, "tcp://")) { /* HACK: source host generates bogus URIs, so fix them up */ @@ -10779,6 +10785,15 @@ qemudDomainMigratePerform (virDomainPtr dom, int resume = 0; qemuDomainObjPrivatePtr priv; + virCheckFlags(VIR_MIGRATE_LIVE | + VIR_MIGRATE_PEER2PEER | + VIR_MIGRATE_TUNNELLED | + VIR_MIGRATE_PERSIST_DEST | + VIR_MIGRATE_UNDEFINE_SOURCE | + VIR_MIGRATE_PAUSED | + VIR_MIGRATE_NON_SHARED_DISK | + VIR_MIGRATE_NON_SHARED_INC, -1); + qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); if (!vm) { @@ -10882,6 +10897,15 @@ qemudDomainMigrateFinish2 (virConnectPtr dconn, virErrorPtr orig_err; int newVM = 1; + virCheckFlags(VIR_MIGRATE_LIVE | + VIR_MIGRATE_PEER2PEER | + VIR_MIGRATE_TUNNELLED | + VIR_MIGRATE_PERSIST_DEST | + VIR_MIGRATE_UNDEFINE_SOURCE | + VIR_MIGRATE_PAUSED | + VIR_MIGRATE_NON_SHARED_DISK | + VIR_MIGRATE_NON_SHARED_INC, NULL); + /* Migration failed. Save the current error so nothing squashes it */ orig_err = virSaveLastError(); diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 5511550..4b1e2ec 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -1144,6 +1144,7 @@ static int qemuMonitorTextMigrate(qemuMonitorPtr mon, int ret = -1; char *safedest = qemuMonitorEscapeArg(dest); virBuffer extra = VIR_BUFFER_INITIALIZER; + char *extrastr = NULL; if (!safedest) { virReportOOMError(); @@ -1159,10 +1160,12 @@ static int qemuMonitorTextMigrate(qemuMonitorPtr mon, if (virBufferError(&extra)) { virBufferFreeAndReset(&extra); virReportOOMError(); - free(safedest); - return -1; + goto cleanup; } - if (virAsprintf(&cmd, "migrate %s\"%s\"", virBufferContentAndReset(&extra), safedest) < 0) { + + extrastr = virBufferContentAndReset(&extra); + if (virAsprintf(&cmd, "migrate %s\"%s\"", extrastr ? extrastr : "", + safedest) < 0) { virReportOOMError(); goto cleanup; } @@ -1191,6 +1194,7 @@ static int qemuMonitorTextMigrate(qemuMonitorPtr mon, ret = 0; cleanup: + VIR_FREE(extrastr); VIR_FREE(safedest); VIR_FREE(info); VIR_FREE(cmd); -- 1.6.6.1
On 05/25/2010 08:33 AM, Chris Lalancette wrote:
Basic live migration was broken by the commit that added non-shared block support in two ways:
1) It added a virCheckFlags() to doNativeMigrate(). Besides the fact that typical usage of virCheckFlags() is in driver entry points, and doNativeMigrate() is not an entry point, it was missing important flags like VIR_MIGRATE_LIVE. Move the virCheckFlags to the top-level qemuDomainMigratePrepare2 and friends.
2) It also added a memory leak in qemuMonitorTextMigrate() by not freeing the memory used by virBufferContentAndReset(). This is fixed by storing the pointer in a temporary variable and freeing it at the end.
With this patch in place, normal live migration works again.
ACK.
v3: Instead of the churn for virCheckFlagsUI and UL, instead always promote flags to an unsigned long and always use %lx for the fprintf.
Yeah, that is much nicer. And it is easier to extend to unsigned long long if we ever need it in the future. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/25/2010 10:48 AM, Eric Blake wrote:
On 05/25/2010 08:33 AM, Chris Lalancette wrote:
Basic live migration was broken by the commit that added non-shared block support in two ways:
1) It added a virCheckFlags() to doNativeMigrate(). Besides the fact that typical usage of virCheckFlags() is in driver entry points, and doNativeMigrate() is not an entry point, it was missing important flags like VIR_MIGRATE_LIVE. Move the virCheckFlags to the top-level qemuDomainMigratePrepare2 and friends.
2) It also added a memory leak in qemuMonitorTextMigrate() by not freeing the memory used by virBufferContentAndReset(). This is fixed by storing the pointer in a temporary variable and freeing it at the end.
With this patch in place, normal live migration works again.
ACK.
v3: Instead of the churn for virCheckFlagsUI and UL, instead always promote flags to an unsigned long and always use %lx for the fprintf.
Yeah, that is much nicer. And it is easier to extend to unsigned long long if we ever need it in the future.
Right, my thought too. Thanks, I've pushed this. -- Chris Lalancette
Chris Lalancette wrote:
Basic live migration was broken by the commit that added non-shared block support in two ways:
1) It added a virCheckFlags() to doNativeMigrate(). Besides ... diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 77e71cc..941b482 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10175,6 +10175,15 @@ qemudDomainMigratePrepare2 (virConnectPtr dconn, int ret = -1; int internalret;
+ virCheckFlags(VIR_MIGRATE_LIVE | + VIR_MIGRATE_PEER2PEER | + VIR_MIGRATE_TUNNELLED | + VIR_MIGRATE_PERSIST_DEST | + VIR_MIGRATE_UNDEFINE_SOURCE | + VIR_MIGRATE_PAUSED | + VIR_MIGRATE_NON_SHARED_DISK | + VIR_MIGRATE_NON_SHARED_INC, -1);
Hi Chris, This looks like a fine change, but I haven't delved into it enough to give a formal ACK. However, one quick comment: It was not at all obvious to me that those three lists of eight VIR_* macros or'd together were identical. I used the editor to confirm it. Considering that someone might be tempted to modify one but miss the other two -- or add a 4th use, would it make sense to define a new symbol, and then use that in those three calls? #define VIR_MIGRATE_SOMETHING \ (VIR_MIGRATE_LIVE | \ VIR_MIGRATE_PEER2PEER | \ VIR_MIGRATE_TUNNELLED | \ VIR_MIGRATE_PERSIST_DEST | \ VIR_MIGRATE_UNDEFINE_SOURCE | \ VIR_MIGRATE_PAUSED | \ VIR_MIGRATE_NON_SHARED_DISK | \ VIR_MIGRATE_NON_SHARED_INC) ...
+ virCheckFlags(VIR_MIGRATE_LIVE | + VIR_MIGRATE_PEER2PEER | + VIR_MIGRATE_TUNNELLED | + VIR_MIGRATE_PERSIST_DEST | + VIR_MIGRATE_UNDEFINE_SOURCE | + VIR_MIGRATE_PAUSED | + VIR_MIGRATE_NON_SHARED_DISK | + VIR_MIGRATE_NON_SHARED_INC, -1); ... + virCheckFlags(VIR_MIGRATE_LIVE | + VIR_MIGRATE_PEER2PEER | + VIR_MIGRATE_TUNNELLED | + VIR_MIGRATE_PERSIST_DEST | + VIR_MIGRATE_UNDEFINE_SOURCE | + VIR_MIGRATE_PAUSED | + VIR_MIGRATE_NON_SHARED_DISK | + VIR_MIGRATE_NON_SHARED_INC, NULL);
On 05/25/2010 11:08 AM, Jim Meyering wrote:
Hi Chris,
This looks like a fine change, but I haven't delved into it enough to give a formal ACK. However, one quick comment: It was not at all obvious to me that those three lists of eight VIR_* macros or'd together were identical. I used the editor to confirm it. Considering that someone might be tempted to modify one but miss the other two -- or add a 4th use, would it make sense to define a new symbol, and then use that in those three calls?
#define VIR_MIGRATE_SOMETHING \ (VIR_MIGRATE_LIVE | \ VIR_MIGRATE_PEER2PEER | \ VIR_MIGRATE_TUNNELLED | \ VIR_MIGRATE_PERSIST_DEST | \ VIR_MIGRATE_UNDEFINE_SOURCE | \ VIR_MIGRATE_PAUSED | \ VIR_MIGRATE_NON_SHARED_DISK | \ VIR_MIGRATE_NON_SHARED_INC)
It's not a terrible idea. The good news is that from a testing perspective, if anyone were to add a VIR_MIGRATE_FOO flag, and put it into MigratePrepare and not MigratePerform, their testing would fail and they would have to investigate. On the other hand, it's much better to let them do things correctly from the get-go. I'll look at making a more generic macro for this. -- Chris Lalancette
participants (3)
-
Chris Lalancette -
Eric Blake -
Jim Meyering