"Dr. David Alan Gilbert" <dgilb...@redhat.com> wrote:
> * Peter Xu (pet...@redhat.com) wrote:
>> There are some places that binded "return path" with postcopy. Let's be
>> prepared for its usage even without postcopy. This patch mainly did this
>> on source side.
>>
>> Signed-off-by: Peter Xu <pet...@redhat.com>
>> ---
>> standalone patch isolated from the return path series. ok to be picked
>> up in case one day we'll re-face the return path enablement.
>> With it, we are ready on source side. The dst side change has been queued.
>> ---
>> migration/migration.c | 11 ++++++-----
>> migration/trace-events | 4 ++--
>> 2 files changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index ad29e53..96e549e 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1850,13 +1850,12 @@ static void migration_completion(MigrationState *s,
>> int current_active_state,
>> * cleaning everything else up (since if there are no failures
>> * it will wait for the destination to send it's status in
>> * a SHUT command).
>> - * Postcopy opens rp if enabled (even if it's not avtivated)
>> */
>> - if (migrate_postcopy_ram()) {
>> + if (s->rp_state.from_dst_file) {
>> int rp_error;
>> - trace_migration_completion_postcopy_end_before_rp();
>> + trace_migration_return_path_end_before();
>> rp_error = await_return_path_close_on_source(s);
>> - trace_migration_completion_postcopy_end_after_rp(rp_error);
>> + trace_migration_return_path_end_after(rp_error);
>> if (rp_error) {
>> goto fail_invalidate;
>> }
>> @@ -1931,13 +1930,15 @@ static void *migration_thread(void *opaque)
>>
>> qemu_savevm_state_header(s->to_dst_file);
>>
>> - if (migrate_postcopy_ram()) {
>> + if (s->to_dst_file) {
>> /* Now tell the dest that it should open its end so it can reply */
>> qemu_savevm_send_open_return_path(s->to_dst_file);
>>
>> /* And do a ping that will make stuff easier to debug */
>> qemu_savevm_send_ping(s->to_dst_file, 1);
>
> I'm confused.
> The migration_thread on the source will always have a s->to_dst_file
> there so why test?
>
> But also, we can't send the 'open return path' and 'send ping' messages
> without a guard; sending them to old QEMUs will cause them to fail when
> they don't know what the message is. I suspect sending them to
> slightly-old QEMUs will cause them to fail when they try and send a
> return message if the destination can't send it.
>
> I can see a if (s->rp_state.from_dst_file) making some sense if it's
> done at the right point.
You are right.
Second chunk makes no sense. I told Peter to split the
"objectification" of migration state and this patch. This made more
sense when the user *had* required a return path.
Good catch.
Thanks, Juan.
