Re: [Qemu-devel] [V4 PATCH 2/5] net: announce self after vm start

[Jason Wang <jasowang@redhat.com>]

On 03/13/2012 05:18 PM, Paolo Bonzini wrote:
> Il 13/03/2012 09:56, Jason Wang ha scritto:
>> This patch moves qemu_announce_self() to vm_start() and add a new parameters to
>> control whether sending gratuitous packet is needed. There are several reasons
>> to do this:
>>
>> - Gratuitous packet is also needed when we resume a stopped vm or successfully
>>    load a state.
>> - Sending gratuitous packets may be done through co-operation with guest, so
>>    this work should be done after vm is started.
>>
>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>> ---
>>   gdbstub.c   |    2 +-
>>   migration.c |    5 ++---
>>   monitor.c   |    2 +-
>>   qmp.c       |    2 +-
>>   savevm.c    |    2 +-
>>   sysemu.h    |    2 +-
>>   vl.c        |    7 +++++--
>>   7 files changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index ef95ac2..f4d22e5 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -371,7 +371,7 @@ static inline void gdb_continue(GDBState *s)
>>   #ifdef CONFIG_USER_ONLY
>>       s->running_state = 1;
>>   #else
>> -    vm_start();
>> +    vm_start(false);
>>   #endif
> Here we're switching from RUNSTATE_DEBUG.
>
>>   }
>>
>> diff --git a/migration.c b/migration.c
>> index 00fa1e3..40332d2 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -88,14 +88,13 @@ void process_incoming_migration(QEMUFile *f)
>>           fprintf(stderr, "load of migration failed\n");
>>           exit(0);
>>       }
>> -    qemu_announce_self();
>>       DPRINTF("successfully loaded vm state\n");
>>
>>       /* Make sure all file formats flush their mutable metadata */
>>       bdrv_invalidate_cache_all();
>>
>>       if (autostart) {
>> -        vm_start();
>> +        vm_start(true);
> Here from RUN_STATE_INMIGRATE.
>
>>       } else {
>>           runstate_set(RUN_STATE_PRELAUNCH);
>>       }
>> @@ -274,7 +273,7 @@ static void migrate_fd_put_ready(void *opaque)
>>           }
>>           if (s->state != MIG_STATE_COMPLETED) {
>>               if (old_vm_running) {
>> -                vm_start();
>> +                vm_start(false);
> Here from RUN_STATE_FINISH_MIGRATE.
>
>>               }
>>           }
>>       }
>> diff --git a/monitor.c b/monitor.c
>> index cbdfbad..0b63c11 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -2260,7 +2260,7 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
>>       vm_stop(RUN_STATE_RESTORE_VM);
>>
>>       if (load_vmstate(name) == 0&&  saved_vm_running) {
>> -        vm_start();
>> +        vm_start(true);
> Here from RUN_STATE_RESTORE_VM.
>
>>       }
>>   }
>>
>> diff --git a/qmp.c b/qmp.c
>> index a182b51..252c842 100644
>> --- a/qmp.c
>> +++ b/qmp.c
>> @@ -160,7 +160,7 @@ void qmp_cont(Error **errp)
>>           return;
>>       }
>>
>> -    vm_start();
>> +    vm_start(true);
> Here from RUN_STATE_PAUSED or RUN_STATE_PRELAUNCH.
>
> This introduces a difference here with "qemu -S" + cont, and "qemu".
> The former will send a gratuitous ARP, the latter won't.  Is this
> desired/harmless/...?

Not desired but harmless I think.

And indeed there're two possibilities:

- start a fresh vm with -S, and then continue the vm.
- migrate a guest with -S option used in destination, and then continue 
the vm.

It's a little hard to differentiate one from another just in vm_stop().
>
>>   }
>>
>>   void qmp_system_wakeup(Error **errp)
>> diff --git a/savevm.c b/savevm.c
>> index 5b59826..82b9d3a 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -2107,7 +2107,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>>
>>    the_end:
>>       if (saved_vm_running)
>> -        vm_start();
>> +        vm_start(false);
>>   }
> This is RUN_STATE_SAVE_VM.
>
>>
>>   int load_vmstate(const char *name)
>> diff --git a/sysemu.h b/sysemu.h
>> index 98118cc..787edd4 100644
>> --- a/sysemu.h
>> +++ b/sysemu.h
>> @@ -34,7 +34,7 @@ void vm_state_notify(int running, RunState state);
>>   #define VMRESET_SILENT   false
>>   #define VMRESET_REPORT   true
>>
>> -void vm_start(void);
>> +void vm_start(bool announce);
>>   void vm_stop(RunState state);
>>   void vm_stop_force_state(RunState state);
>>
>> diff --git a/vl.c b/vl.c
>> index 65f11f2..4e04f82 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1258,7 +1258,7 @@ void vm_state_notify(int running, RunState state)
>>       }
>>   }
>>
>> -void vm_start(void)
>> +void vm_start(bool announce)
>>   {
>>       if (!runstate_is_running()) {
>>           cpu_enable_ticks();
>> @@ -1266,6 +1266,9 @@ void vm_start(void)
>>           vm_state_notify(1, RUN_STATE_RUNNING);
>>           resume_all_vcpus();
>>           monitor_protocol_event(QEVENT_RESUME, NULL);
>> +        if (announce) {
>> +            qemu_announce_self();
>> +        }
>>       }
>>   }
>>
>> @@ -3619,7 +3622,7 @@ int main(int argc, char **argv, char **envp)
>>               exit(ret);
>>           }
>>       } else if (autostart) {
>> -        vm_start();
>> +        vm_start(false);
>>       }
> This is RUN_STATE_PRELAUNCH.
>
> To some up, it seems like whether to send an announcement depends on the
> previous runstate: it should be sent only for RUN_STATE_INMIGRATE,
> RUN_STATE_RESTORE_VM and (new with your patch) RUN_STATE_PAUSED.  So
> perhaps the new argument to vm_start is not needed.
>
> Paolo

Make sense, thanks.
>>
>>       os_setup_post();
>>
>>
>>