Re: [PATCH v4 11/25] migration: Add Error** argument to .save_setup() handler

[Cédric Le Goater]

On 3/7/24 10:53, Vladimir Sementsov-Ogievskiy wrote:
> On 06.03.24 16:34, Cédric Le Goater wrote:
> > The purpose is to record a potential error in the migration stream if
> > qemu_savevm_state_setup() fails. Most of the current .save_setup()
> > handlers can be modified to use the Error argument instead of managing
> > their own and calling locally error_report().
> >
> > Cc: Nicholas Piggin <npiggin@gmail.com>
> > Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
> > Cc: Halil Pasic <pasic@linux.ibm.com>
> > Cc: Thomas Huth <thuth@redhat.com>
> > Cc: Eric Blake <eblake@redhat.com>
> > Cc: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > Cc: John Snow <jsnow@redhat.com>
> > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> > Reviewed-by: Thomas Huth <thuth@redhat.com>
> > Signed-off-by: Cédric Le Goater <clg@redhat.com>
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
> Still, if you resend, please add error_prepend in the case below:
>
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 
> > 63fdbb5ad7d4dbfaef1d2094350bf302cc677602..52d35b2a72c6238bfe5dcb4d81c1af8d2bf73013
> >  100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -1342,11 +1342,9 @@ int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
> >           }
> >           save_section_header(f, se, QEMU_VM_SECTION_START);
> > -        ret = se->ops->save_setup(f, se->opaque);
> > +        ret = se->ops->save_setup(f, se->opaque, errp);
> >           save_section_footer(f, se);
> >           if (ret < 0) {
> > -            error_setg(errp, "failed to setup SaveStateEntry with id(name): "
> > -                       "%d(%s): %d", se->section_id, se->idstr, ret);
>
> You drop a good bit of information, let's use error_prepend to save it.

I kind of agree but the call stack is quite deep and the callees also use
error_prepend. The error becomes quite long. Here's an example of what we
get today :

  (qemu) migrate -d tcp:10.8.3.15:1234
  (qemu)
  (qemu) qemu-system-x86_64: vfio: Could not start dirty page tracking - 
0000:b1:00.2: Failed to start DMA logging: Invalid argument

If the subsystems implementing a .save_setup() handler use a component
identifier, the failure should be fairly easy to identify.

What's the best practice for such cases ?

Can we use multiline errors maybe ? Less practical for grep though.

May be a verbose error mode would help getting more information ?

Anyhow, I can add a new trace event for "failed to setup SaveStateEntry ... "
or use error_prepend() as you suggested.

Let's see what the others have to say.


> >               qemu_file_set_error(f, ret);
> >               break;
>
> Not about this patch:
>
> Better do explicit "return ret" instead of this "break" (and one more break 
> above in that loop):
>
> 1. making a jump to do just do "return ret" seems overkill. It would make sense if we had some more 
> "cleanup" code than just a "return ret", and if so, more classic and readable thing is "goto 
> fail;".
> 2. "break" make me think, that there may be more logic after it, which will probably 
> fail, and I should be careful, as errp is already set (and second attempt to set it will crash). 
> Again, "goto fail;" is better, as I don't expect more failures when see it.

Sure. If I respin, I can drop the break and simply return. Although,
I would be glad to have most of this series merged in QEMU 9.0. So,
unless there is something major, I will keep that for followups.


Thanks for the review,

C.