[Qemu-devel] Re: [PATCH] migration: Issue 'cont' only on successful incoming migration

[Juan Quintela]

Laine Stump <address@hidden> wrote:
>  On 07/26/2010 10:23 AM, Luiz Capitulino wrote:
>> On Sat, 24 Jul 2010 13:01:24 +0530
>> Amit Shah<address@hidden>  wrote:
>>
>>> On (Fri) Jul 23 2010 [15:08:18], Luiz Capitulino wrote:
>>>>> diff --git a/monitor.c b/monitor.c
>>>>> index 45fd482..d12a7b5 100644
>>>>> --- a/monitor.c
>>>>> +++ b/monitor.c
>>>>> @@ -1056,6 +1056,10 @@ static int do_cont(Monitor *mon, const QDict 
>>>>> *qdict, QObject **ret_data)
>>>>>   {
>>>>>       struct bdrv_iterate_context context = { mon, 0 };
>>>>>
>>>>> +    if (incoming_expected&&  !incoming_done) {
>>>>> +        autostart = 1;
>>>> Why do we need to set autostart? We should just fail if we're unable to 
>>>> run.
>>>>
>>>>> +        return 1; /* Waiting for incoming migration */
>>>> You should return -1 and use qerror_report(), so that we have a meaningful
>>>> error in the user Monitor and QMP (otherwise we'll get UndefinedError).
>>> That would mean old/existing libvirt will be confused on why guests
>>> wouldn't start even though it issued cont.
>> Yes, although delaying to start could cause a problem too and this is
>> also introducing an new error in QMP already.
>>
>> I really would like to avoid adding weird semantics, specially in QMP where
>> cont will return an error but will put the VM to run later. We could fix this
>> there only, but then it will get complex w/o reason.
>>
>> We should fix it properly right now, IMO.
>>
>>> If it's not a problem for the libvirt folks, I can do that.
>> Laine, could you please check that?
>
> That should really be answered by someone who better understands the
> implications (I'm a newcomer to that part of the code). Dan Berrange
> or Chris Lalancette maybe?
>
> (I am setting up to test the current version of the patch on the
> system where I can reproduce the problem. Haven't flipped the switch
> yet, though.)

Just to be sure, what do you want "cont" to return if we are in the
middle of a migration?

This patch just sets autostart=1 (i.e. user wants to start guest as soon
as possible), is that ok, or a warning/error is better?

Later, Juan.