Re: [Qemu-devel] [PATCH RFC v2 4/5] migration: remove unused &local_err parameter in multifd_save_cleanup

[Fei Li]

On 11/29/2018 10:50 PM, Philippe Mathieu-Daudé wrote:
> On 29/11/18 11:03, Fei Li wrote:
> > Always call migrate_set_error() to set the error state without relying
> > on whether multifd_save_cleanup() succeeds.  As the passed &local_err
> > is never used in multifd_save_cleanup(), remove it. And make the
> > function be: void multifd_save_cleanup(void).
> Reading this after your patch 1/5, maybe multifd_save_cleanup() lacks
> error reporting on failure. So the actual prototype is correct, we just
> need to use **errp in that function (like reporting invalid thread
> instead of calling qemu_thread_join()).
Sorry that maybe the patch sequences causethe mis-understanding.
The patch "2/5: qemu_thread_join: fix segmentation fault" is added to
fix another problem when qemu_thread_create() fails.

The qemu_thread_join() => pthread_join in multifd_save_cleanup() is
used to terminate the already created/running thread, so I do not
think it should be replaced. Besides, do we need to report the
invalid thread for users?
But for multifd_save_cleanup() itself, it is called when
1. fails to send one multifd channel. In this way, the error reason has
    been stored in MigrationState->error, but not be printed. I think we
    can add a `error_report_err(local_err)` in 
multifd_new_send_channel_async()
    just as follows:

@@ -1070,6 +1070,7 @@ static void multifd_new_send_channel_async(QIOTask 
*task, gpointer opaque)
     if (qio_task_propagate_error(task, &local_err)) {
         migrate_set_error(migrate_get_current(), local_err);
+        error_report_err(local_err);
         multifd_save_cleanup();
     } else {

BTW, we also need to notify the main thread that the migrate_state
is failed in this case, but currently this issue is still under discussion.
Please see patch 5/9 in"[PATCH RFC v7 0/9] qemu_thread_create:
propagate errors to callers to check." for more details. :)
2. migrate_fd_cleanup() is called to do the cleanup (In this way, the real
    error reason is stored and also will be printed inside in 
migrate_fd_cleanup()
    or before migrate_fd_cleanup() is called. Thus IMO no update is 
needed here.)

Have a nice day, thanks
Fei
> > Signed-off-by: Fei Li <address@hidden>
> > Reviewed-by: Juan Quintela <address@hidden>
> > ---
> >   migration/migration.c |  5 +----
> >   migration/ram.c       | 11 ++++-------
> >   migration/ram.h       |  2 +-
> >   3 files changed, 6 insertions(+), 12 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 72106bddf0..0537fc0c26 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -1386,7 +1386,6 @@ static void migrate_fd_cleanup(void *opaque)
> >       qemu_savevm_state_cleanup();
> >   
> >       if (s->to_dst_file) {
> > -        Error *local_err = NULL;
> >           QEMUFile *tmp;
> >   
> >           trace_migrate_fd_cleanup();
> > @@ -1397,9 +1396,7 @@ static void migrate_fd_cleanup(void *opaque)
> >           }
> >           qemu_mutex_lock_iothread();
> >   
> > -        if (multifd_save_cleanup(&local_err) != 0) {
> > -            error_report_err(local_err);
> > -        }
> > +        multifd_save_cleanup();
> >           qemu_mutex_lock(&s->qemu_file_lock);
> >           tmp = s->to_dst_file;
> >           s->to_dst_file = NULL;
> > diff --git a/migration/ram.c b/migration/ram.c
> > index e13b9349d0..c44cb6f56d 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -917,13 +917,12 @@ static void multifd_send_terminate_threads(Error *err)
> >       }
> >   }
> >   
> > -int multifd_save_cleanup(Error **errp)
> > +void multifd_save_cleanup(void)
> >   {
> >       int i;
> > -    int ret = 0;
> >   
> >       if (!migrate_use_multifd()) {
> > -        return 0;
> > +        return;
> >       }
> >       multifd_send_terminate_threads(NULL);
> >       for (i = 0; i < migrate_multifd_channels(); i++) {
> > @@ -953,7 +952,6 @@ int multifd_save_cleanup(Error **errp)
> >       multifd_send_state->pages = NULL;
> >       g_free(multifd_send_state);
> >       multifd_send_state = NULL;
> > -    return ret;
> >   }
> >   
> >   static void multifd_send_sync_main(void)
> > @@ -1071,9 +1069,8 @@ static void multifd_new_send_channel_async(QIOTask *task, 
> > gpointer opaque)
> >       Error *local_err = NULL;
> >   
> >       if (qio_task_propagate_error(task, &local_err)) {
> > -        if (multifd_save_cleanup(&local_err) != 0) {
> > -            migrate_set_error(migrate_get_current(), local_err);
> > -        }
> > +        migrate_set_error(migrate_get_current(), local_err);
> > +        multifd_save_cleanup();
> >       } else {
> >           p->c = QIO_CHANNEL(sioc);
> >           qio_channel_set_delay(p->c, false);
> > diff --git a/migration/ram.h b/migration/ram.h
> > index 046d3074be..936177b3e9 100644
> > --- a/migration/ram.h
> > +++ b/migration/ram.h
> > @@ -43,7 +43,7 @@ uint64_t ram_bytes_remaining(void);
> >   uint64_t ram_bytes_total(void);
> >   
> >   int multifd_save_setup(void);
> > -int multifd_save_cleanup(Error **errp);
> > +void multifd_save_cleanup(void);
> >   int multifd_load_setup(void);
> >   int multifd_load_cleanup(Error **errp);
> >   bool multifd_recv_all_channels_created(void);