Re: [PATCH v4 13/25] memory: Add Error** argument to .log_global_start() handler

[Cédric Le Goater]

On 3/15/24 12:18, Peter Xu wrote:
> > @@ -3009,13 +3045,16 @@ static void listener_add_address_space(MemoryListener 
> > *listener,
> >   {
> >       FlatView *view;
> >       FlatRange *fr;
> > +    Error *local_err = NULL;
> >   
> >       if (listener->begin) {
> >           listener->begin(listener);
> >       }
> >       if (global_dirty_tracking) {
> >           if (listener->log_global_start) {
> > -            listener->log_global_start(listener);
> > +            if (!listener->log_global_start(listener, &local_err)) {
> > +                error_report_err(local_err);
> > +            }
> IMHO we should assert here instead of error report.  We have this to guard
> hot-plug during migration so I think the assert is justified:
>
> qdev_device_add_from_qdict():
>
>      if (!migration_is_idle()) {
>          error_setg(errp, "device_add not allowed while migrating");
>          return NULL;
>      }
>
> If it really happens it's a bug, as listener_add_address_space() will still
> keep the rest things around even if the hook failed.  It'll start to be a
> total mess..

It seems that adding a region listener while logging is active has been
supported from the beginning, commit 7664e80c8470 ("memory: add API for
observing  updates to the physical memory map"). Can it happen ? if not
we could simply remove the  log_global_start() call.

Thanks,

C.