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

[Laine Stump <laine@redhat.com>]

  On 07/26/2010 10:23 AM, Luiz Capitulino wrote:
> On Sat, 24 Jul 2010 13:01:24 +0530
> Amit Shah<amit.shah@redhat.com>  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.)