Re: [Qemu-devel] [PATCH RFC 4/6] migration: shut src return path unconditionally

[Peter Xu]

On Tue, May 30, 2017 at 05:50:27PM +0200, Juan Quintela wrote:
> Peter Xu <address@hidden> wrote:
> > We were do the shutting off only for postcopy. Now we do this as long as
> > the source return path is there.
> >
> > Moving the cleanup of from_src_file there too.
> >
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> >  migration/migration.c    | 8 +++++++-
> >  migration/postcopy-ram.c | 1 -
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 92617fc..a4006b4 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -131,10 +131,17 @@ void migration_incoming_state_destroy(void)
> >      struct MigrationIncomingState *mis = migration_incoming_get_current();
> >  
> >      if (mis->to_src_file) {
> > +        /* Tell source that we are done */
> > +        migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) 
> > != 0);
> 
> Reviewed-by: Juan Quintela <address@hidden>
> 
> 
> I think this one belongs to previous patch (with accompaining line from 
> below).
> But just if you want to change it.

I separated it since these two patches were actually doing different
things:

- previous patch fixed one possible leak, while

- this patch postponed MIG_RP_MSG_SHUT a bit to the end, and let it
  not depending on postcopy, but the return path itself (so that we
  can enable the return path even without postcopy then)

Meanwhile, there might be problem if we just put this single line into
previous patch, since this line depends on below change [1]
(from_src_file should better be closed after this
qemu_file_get_error() call). So... I would still prefer to separate
them using current way. Even if we really want to merge them, I would
prefer directly squashing current patch into previous one.

Thanks,

> 
> >          qemu_fclose(mis->to_src_file);
> >          mis->to_src_file = NULL;
> >      }
> >  
> > +    if (mis->from_src_file) {
> > +        qemu_fclose(mis->from_src_file);
> > +        mis->from_src_file = NULL;
> > +    }
> > +

[1]

> >      qemu_event_destroy(&mis->main_thread_load_event);
> >      loadvm_free_handlers(mis);
> >  }
> > @@ -433,7 +440,6 @@ static void process_incoming_migration_co(void *opaque)
> >          exit(EXIT_FAILURE);
> >      }
> >  
> > -    qemu_fclose(f);
> >      free_xbzrle_decoded_buf();
> >  
> >      mis->bh = qemu_bh_new(process_incoming_migration_bh, mis);
> > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > index a0489f6..57aa208 100644
> > --- a/migration/postcopy-ram.c
> > +++ b/migration/postcopy-ram.c
> > @@ -320,7 +320,6 @@ int 
> > postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
> >      }
> >  
> >      postcopy_state_set(POSTCOPY_INCOMING_END);
> > -    migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 
> > 0);
> >  
> >      if (mis->postcopy_tmp_page) {
> >          munmap(mis->postcopy_tmp_page, mis->largest_page_size);

-- 
Peter Xu