Re: [libvirt] [PATCH v3 2/2] auto cold migration fallback at timeout

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.

> > Signed-off-by: Wen Congyang <wency(a)cn.fujitsu.com&gt; > > --- > 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).

> > 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)"

> {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.