At 12/18/2010 07:10 AM, Eric Blake Write: > On 12/17/2010 01:49 AM, Wen Congyang wrote: >> implement auto cold migration fallback at timeout > > Maybe it's a language barrier issue thing, but I had to read this patch > several times before I understood what you were getting at. Does this > wording work any better? > > If a guest has not completed live migration before timeout, then > auto-suspend the guest, where the migration will complete offline. Yes. > >> >> Signed-off-by: Wen Congyang <wency@xxxxxxxxxxxxxx> >> >> --- >> tools/virsh.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 66 insertions(+), 0 deletions(-) > > Missing a change to tools/virsh.pod to document the new option (that > wording should be more complete, in comparison to the one-line blurb for > the 'virsh help migrate' output). I will add it. > >> >> diff --git a/tools/virsh.c b/tools/virsh.c >> index cbde085..b95c8fe 100644 >> --- a/tools/virsh.c >> +++ b/tools/virsh.c >> @@ -3379,9 +3379,32 @@ static const vshCmdOptDef opts_migrate[] = { >> {"desturi", VSH_OT_DATA, VSH_OFLAG_REQ, N_("connection URI of the destination host")}, >> {"migrateuri", VSH_OT_DATA, 0, N_("migration URI, usually can be omitted")}, >> {"dname", VSH_OT_DATA, 0, N_("rename to new name during migration (if supported)")}, >> + {"timeout", VSH_OT_INT, 0, N_("auto cold migration fallback when live migration timeouts(in seconds)")}, > > Maybe: "force guest to suspend if live migration exceeds timeout (in > seconds)" OK > >> {NULL, 0, 0, NULL} >> }; >> >> +static void migrateTimeoutHandler(void *data) >> +{ > > I'd float this helper function to live above the cmdMigrate definition; > placing it here interrupts the flow between what options cmdMigrate > takes and how it uses them. > >> + virDomainPtr dom = (virDomainPtr)data; > > Cast is not necessary. > >> cmdMigrate (vshControl *ctl, const vshCmd *cmd) >> { >> @@ -3389,6 +3412,8 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd) >> const char *desturi; >> const char *migrateuri; >> const char *dname; >> + long long timeout; >> + virTimerPtr migratetimer = NULL; >> int flags = 0, found, ret = FALSE; >> >> if (!vshConnectionUsability (ctl, ctl->conn)) >> @@ -3426,6 +3451,28 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd) >> if (vshCommandOptBool (cmd, "copy-storage-inc")) >> flags |= VIR_MIGRATE_NON_SHARED_INC; >> >> + timeout = vshCommandOptLongLong(cmd, "timeout", &found); > > Why long long? No one is going to set a timeout bigger than 4 billion > seconds, so plain int would do just fine here. In particular, since > virEventAddTimeout can only sleep up to int milliseconds, and you are > already multiplying the user's timeout value (in seconds) by 1000, you > already have to worry about overflow issues - if the user requests 5 > million seconds (which overflows 4 billion milliseconds for the > underlying timer max frequency), you must reject the user's value. > >> + >> + migratetimer = virNewTimer(migrateTimeoutHandler, (void *)dom); >> + if (!migratetimer) >> + goto done; >> + >> + if (virStartTimer() < 0) { > > Hmm - given your usage of starting a timer thread as long as any > virTimerPtr object exists, maybe you're better off creating some sort of > global ref-counting state under the hood of timer.c that tracks how many > virTimer objects are currently set to have a time. The user should only > have to call a single function to create their timer object, without > having to make extra calls to feed your underlying implementation. > >> @@ -3455,6 +3516,11 @@ cmdMigrate (vshControl *ctl, const vshCmd *cmd) >> } >> >> done: >> + if (migratetimer) { >> + virStopTimer(); >> + virDelTimer(migratetimer); > > Again, if multiple threads are managing timers, then the user shouldn't > be able to kill the timer resources just because one thread no longer > needs a timer. The user shouldn't have to call virStopTimer. > Thanks for your comment. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list