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

[Wen Congyang]

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 at cn.fujitsu.com>
>>
>> ---
>>  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.