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

[Peter Xu]

On Mon, Mar 18, 2024 at 03:54:28PM +0100, Cédric Le Goater wrote:
> 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.

IMHO we'd better keep it for the sake of logic completeness, even though I
don't know when it'll be useful..

I think it's safe to assert because log_global_start() should only be
triggered by either vhost/vfio with current code base when reaching here.
It doesn't mean that in the future all log_global_start() hooks are based
on a device object. E.g., there's the other Xen user, it just won't trigger
either, afaict.  So the assert should be safe.

In the future maybe we could allow other things to trigger here besides
device, but obviously we're not ready for failing it.  Instead of adding
the failure handling which will never be used for now, IIUC it's simpler we
just provide an assert until someone add a real user of such.

Thanks,

-- 
Peter Xu