Re: [PATCH 2/3] migration: Simplify calling of await_return_path_close_on_source

[Peter Xu <peterx@redhat.com>]

On Fri, Jul 28, 2023 at 09:15:15AM -0300, Fabiano Rosas wrote:
> We're about to reuse this function so move the 'rp_thread_created'
> check into it and remove the redundant tracing and comment.
> 
> Add a new tracepoint akin to what is already done at
> migration_completion().
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/migration.c  | 21 +++++++--------------
>  migration/trace-events |  3 +--
>  2 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 051067f8c5..d6f4470265 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2038,6 +2038,10 @@ static int open_return_path_on_source(MigrationState *ms,
>  /* Returns 0 if the RP was ok, otherwise there was an error on the RP */
>  static int await_return_path_close_on_source(MigrationState *ms)
>  {
> +    if (!ms->rp_state.rp_thread_created) {
> +        return 0;
> +    }
> +
>      /*
>       * If this is a normal exit then the destination will send a SHUT and the
>       * rp_thread will exit, however if there's an error we need to cause
> @@ -2350,20 +2354,9 @@ static void migration_completion(MigrationState *s)
>          goto fail;
>      }
>  
> -    /*
> -     * If rp was opened we must clean up the thread before
> -     * 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).
> -     */
> -    if (s->rp_state.rp_thread_created) {
> -        int rp_error;
> -        trace_migration_return_path_end_before();
> -        rp_error = await_return_path_close_on_source(s);
> -        trace_migration_return_path_end_after(rp_error);
> -        if (rp_error) {
> -            goto fail;
> -        }
> +    if (await_return_path_close_on_source(s)) {
> +        trace_migration_completion_rp_err();
> +        goto fail;
>      }
>  
>      if (qemu_file_get_error(s->to_dst_file)) {
> diff --git a/migration/trace-events b/migration/trace-events
> index 5259c1044b..33a69064ca 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -157,13 +157,12 @@ migrate_pending_estimate(uint64_t size, uint64_t pre, uint64_t post) "estimate p
>  migrate_send_rp_message(int msg_type, uint16_t len) "%d: len %d"
>  migrate_send_rp_recv_bitmap(char *name, int64_t size) "block '%s' size 0x%"PRIi64
>  migration_completion_file_err(void) ""
> +migration_completion_rp_err(void) ""
>  migration_completion_vm_stop(int ret) "ret %d"
>  migration_completion_postcopy_end(void) ""
>  migration_completion_postcopy_end_after_complete(void) ""
>  migration_rate_limit_pre(int ms) "%d ms"
>  migration_rate_limit_post(int urgent) "urgent: %d"
> -migration_return_path_end_before(void) ""
> -migration_return_path_end_after(int rp_error) "%d"
>  migration_thread_after_loop(void) ""
>  migration_thread_file_err(void) ""
>  migration_thread_setup_complete(void) ""
> -- 
> 2.35.3
> 

Just in case someone may still want to see the old
trace_migration_return_path_end_before() tracepoint, maybe just move them
all over?

-- 
Peter Xu