Re: [PATCH v3 03/10] migration: Refactor error handling in source return path

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> @@ -1882,48 +1870,46 @@ static void *source_return_path_thread(void *opaque)
>      uint32_t tmp32, sibling_error;
>      ram_addr_t start = 0; /* =0 to silence warning */
>      size_t  len = 0, expected_len;
> +    Error *err = NULL;
>      int res;
>  
>      trace_source_return_path_thread_entry();
>      rcu_register_thread();
>  
> -    while (!ms->rp_state.error && !qemu_file_get_error(rp) &&
> +    while (!migrate_has_error(ms) && !qemu_file_get_error(rp) &&
>             migration_is_setup_or_active(ms->state)) {
>          trace_source_return_path_thread_loop_top();
> +
>          header_type = qemu_get_be16(rp);
>          header_len = qemu_get_be16(rp);
>  
>          if (qemu_file_get_error(rp)) {
> -            mark_source_rp_bad(ms);
>              goto out;
>          }

This error will be lost because outside the loop we only check for err.

>  
>          if (header_type >= MIG_RP_MSG_MAX ||
>              header_type == MIG_RP_MSG_INVALID) {
> -            error_report("RP: Received invalid message 0x%04x length 0x%04x",
> -                         header_type, header_len);
> -            mark_source_rp_bad(ms);
> +            error_setg(&err, "Received invalid message 0x%04x length 0x%04x",
> +                       header_type, header_len);
>              goto out;
>          }
>  
>          if ((rp_cmd_args[header_type].len != -1 &&
>              header_len != rp_cmd_args[header_type].len) ||
>              header_len > sizeof(buf)) {
> -            error_report("RP: Received '%s' message (0x%04x) with"
> -                         "incorrect length %d expecting %zu",
> -                         rp_cmd_args[header_type].name, header_type, header_len,
> -                         (size_t)rp_cmd_args[header_type].len);
> -            mark_source_rp_bad(ms);
> +            error_setg(&err, "Received '%s' message (0x%04x) with"
> +                       "incorrect length %d expecting %zu",
> +                       rp_cmd_args[header_type].name, header_type, header_len,
> +                       (size_t)rp_cmd_args[header_type].len);
>              goto out;
>          }
>  
>          /* We know we've got a valid header by this point */
>          res = qemu_get_buffer(rp, buf, header_len);
>          if (res != header_len) {
> -            error_report("RP: Failed reading data for message 0x%04x"
> -                         " read %d expected %d",
> -                         header_type, res, header_len);
> -            mark_source_rp_bad(ms);
> +            error_setg(&err, "Failed reading data for message 0x%04x"
> +                       " read %d expected %d",
> +                       header_type, res, header_len);
>              goto out;
>          }
>  
> @@ -1933,8 +1919,7 @@ static void *source_return_path_thread(void *opaque)
>              sibling_error = ldl_be_p(buf);
>              trace_source_return_path_thread_shut(sibling_error);
>              if (sibling_error) {
> -                error_report("RP: Sibling indicated error %d", sibling_error);
> -                mark_source_rp_bad(ms);
> +                error_setg(&err, "Sibling indicated error %d", sibling_error);
>              }
>              /*
>               * We'll let the main thread deal with closing the RP
> @@ -1952,7 +1937,10 @@ static void *source_return_path_thread(void *opaque)
>          case MIG_RP_MSG_REQ_PAGES:
>              start = ldq_be_p(buf);
>              len = ldl_be_p(buf + 8);
> -            migrate_handle_rp_req_pages(ms, NULL, start, len);
> +            migrate_handle_rp_req_pages(ms, NULL, start, len, &err);
> +            if (err) {
> +                goto out;
> +            }
>              break;
>  
>          case MIG_RP_MSG_REQ_PAGES_ID:
> @@ -1967,32 +1955,32 @@ static void *source_return_path_thread(void *opaque)
>                  expected_len += tmp32;
>              }
>              if (header_len != expected_len) {
> -                error_report("RP: Req_Page_id with length %d expecting %zd",
> -                             header_len, expected_len);
> -                mark_source_rp_bad(ms);
> +                error_setg(&err, "Req_Page_id with length %d expecting %zd",
> +                           header_len, expected_len);
> +                goto out;
> +            }
> +            migrate_handle_rp_req_pages(ms, (char *)&buf[13], start, len,
> +                                        &err);
> +            if (err) {
>                  goto out;
>              }
> -            migrate_handle_rp_req_pages(ms, (char *)&buf[13], start, len);
>              break;
>  
>          case MIG_RP_MSG_RECV_BITMAP:
>              if (header_len < 1) {
> -                error_report("%s: missing block name", __func__);
> -                mark_source_rp_bad(ms);
> +                error_setg(&err, "MIG_RP_MSG_RECV_BITMAP missing block name");
>                  goto out;
>              }
>              /* Format: len (1B) + idstr (<255B). This ends the idstr. */
>              buf[buf[0] + 1] = '\0';
> -            if (migrate_handle_rp_recv_bitmap(ms, (char *)(buf + 1))) {
> -                mark_source_rp_bad(ms);
> +            if (migrate_handle_rp_recv_bitmap(ms, (char *)(buf + 1), &err)) {
>                  goto out;
>              }
>              break;
>  
>          case MIG_RP_MSG_RESUME_ACK:
>              tmp32 = ldl_be_p(buf);
> -            if (migrate_handle_rp_resume_ack(ms, tmp32)) {
> -                mark_source_rp_bad(ms);
> +            if (migrate_handle_rp_resume_ack(ms, tmp32, &err)) {
>                  goto out;
>              }
>              break;
> @@ -2008,9 +1996,14 @@ static void *source_return_path_thread(void *opaque)
>      }
>  
>  out:
> -    if (qemu_file_get_error(rp)) {
> +    if (err) {

Need to keep both checks here.

> +        /*
> +         * Collect any error in return-path thread and report it to the
> +         * migration state object.
> +         */
> +        migrate_set_error(ms, err);
> +        error_free(err);
>          trace_source_return_path_thread_bad_end();
> -        mark_source_rp_bad(ms);
>      }
>  
>      trace_source_return_path_thread_end();
> @@ -2036,13 +2029,10 @@ static int open_return_path_on_source(MigrationState *ms)
>      return 0;
>  }