Re: [PATCH v3 1/3] migration: Pass Error object errp into vm state loading functions

[Markus Armbruster <armbru@redhat.com>]

Arun Menon <armenon@redhat.com> writes:

> - This is an incremental step in converting vmstate loading
>   code to report errors.
> - Minimal changes to the signature and body of the following
>   functions are done,
>   - vmstate_load()
>   - vmstate_load_state()
>   - vmstate_subsection_load()
>   - qemu_load_device_state()
>   - qemu_loadvm_state()
>   - qemu_loadvm_section_start_full()
>   - qemu_loadvm_section_part_end()
>   - qemu_loadvm_state_header()
>   - qemu_loadvm_state_main()
> - error_report() has been replaced with error_setg();
>   and in places where error has been already set,
>   error_prepend() is used to not lose information.
>
> Signed-off-by: Arun Menon <armenon@redhat.com>

[...]

> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 1ff7bd9ac425ba67cd5ca7ad97bcf570f9e19abe..056781b1c21e737583f081594d9f88b32adfd674 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -1196,7 +1196,7 @@ extern const VMStateInfo vmstate_info_qlist;
>      }
>  
>  int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> -                       void *opaque, int version_id);
> +                       void *opaque, int version_id, Error **errp);
>  int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>                         void *opaque, JSONWriter *vmdesc);
>  int vmstate_save_state_with_err(QEMUFile *f, const VMStateDescription *vmsd,
> diff --git a/migration/colo.c b/migration/colo.c
> index e0f713c837f5da25d67afbd02ceb6c54024ca3af..808c980b4af3199968771cdc6cca3c2451a2f4a6 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -659,6 +659,7 @@ void migrate_start_colo_process(MigrationState *s)
>  static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
>                        QEMUFile *fb, QIOChannelBuffer *bioc, Error **errp)
>  {
> +    ERRP_GUARD();
>      uint64_t total_size;
>      uint64_t value;
>      Error *local_err = NULL;
       int ret;

       bql_lock();
       vm_stop_force_state(RUN_STATE_COLO);
       bql_unlock();
       trace_colo_vm_state_change("run", "stop");

       /* FIXME: This is unnecessary for periodic checkpoint mode */
       colo_send_message(mis->to_src_file, COLO_MESSAGE_CHECKPOINT_REPLY,
                    &local_err);
       if (local_err) {
           error_propagate(errp, local_err);
           return;
       }

Avoid error_propagate() when you have ERRP_GUARD():

       colo_send_message(mis->to_src_file, COLO_MESSAGE_CHECKPOINT_REPLY,
                    errp);
       if (*errp) {
           return;
       }

See the big comment in qapi/error.h for additional guidance.

       colo_receive_check_message(mis->from_src_file,
                          COLO_MESSAGE_VMSTATE_SEND, &local_err);
       if (local_err) {
           error_propagate(errp, local_err);
           return;
       }

Likewise.  More of the same below, not flagging it again.

> @@ -686,11 +687,13 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
>  
>      bql_lock();
>      cpu_synchronize_all_states();
> -    ret = qemu_loadvm_state_main(mis->from_src_file, mis);
> +    ret = qemu_loadvm_state_main(mis->from_src_file, mis, &local_err);
>      bql_unlock();
>  
>      if (ret < 0) {
> -        error_setg(errp, "Load VM's live state (ram) error");
> +        if (local_err != NULL) {

How can @local_err be null here?

> +            error_prepend(errp, "Load VM's live state (ram) error");

Since nothing has set @errp so far, it should still point to null.
error_prepend() crashes when passed a pointer to null.

> +        }
>          return;

Returns without setting an error, and leaks @local_err.

Can you pass @errp to qemu_loadvm_state_main() and call it a day?

>      }
>  
> @@ -729,9 +732,11 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
>      bql_lock();
>      vmstate_loading = true;
>      colo_flush_ram_cache();
> -    ret = qemu_load_device_state(fb);
> +    ret = qemu_load_device_state(fb, &local_err);
>      if (ret < 0) {
> -        error_setg(errp, "COLO: load device state failed");
> +        if (*errp != NULL) {
> +            error_prepend(errp, "COLO: load device state failed");
> +        }

Since nothing has set @errp so far, it should still point to null, so
this will never prepend anything.

>          vmstate_loading = false;
>          bql_unlock();
>          return;

Returns without setting an error, and leaks @local_err.

Can you pass @errp to qemu_load_device_state()?

> diff --git a/migration/cpr.c b/migration/cpr.c
> index a50a57edca754b50e68fa9c294b3c89791e62ba8..0fb9fadac905c83689eed2b1193b282da679b6ef 100644
> --- a/migration/cpr.c
> +++ b/migration/cpr.c
> @@ -235,7 +235,7 @@ int cpr_state_load(MigrationChannel *channel, Error **errp)
>          return -ENOTSUP;
>      }
>  
> -    ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1);
> +    ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1, errp);

If vmstate_load_state() fails, it should set @errp.

>      if (ret) {
>          error_setg(errp, "vmstate_load_state error %d", ret);

Setting it again will trip error_setv()'s assertion.

>          qemu_fclose(f);

I doubt you tested your changes to the error paths.  This is dangerous.

I did not look at the remainder of this patch.

[...]