Re: [PATCH 03/14] memory: Add Error** argument to .log_global*() handlers

[Avihai Horon]

Hi Cedric,

On 09/02/2024 12:14, Cédric Le Goater wrote:
> External email: Use caution opening links or attachments
>
>
> On 2/8/24 06:48, Peter Xu wrote:
> > On Wed, Feb 07, 2024 at 02:33:36PM +0100, Cédric Le Goater wrote:
> > > @@ -2936,14 +2940,14 @@ void memory_global_dirty_log_start(unsigned 
> > > int flags)
> > >       trace_global_dirty_changed(global_dirty_tracking);
> > >
> > >       if (!old_flags) {
> > > -        MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward);
> > > +        MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward, errp);
> > >           memory_region_transaction_begin();
> > >           memory_region_update_pending = true;
> > >           memory_region_transaction_commit();
> > >       }
> > >   }
> > >
> > > -static void memory_global_dirty_log_do_stop(unsigned int flags)
> > > +static void memory_global_dirty_log_do_stop(unsigned int flags, 
> > > Error **errp)
> > >   {
> > >       assert(flags && !(flags & (~GLOBAL_DIRTY_MASK)));
> > >       assert((global_dirty_tracking & flags) == flags);
> > > @@ -2955,7 +2959,7 @@ static void 
> > > memory_global_dirty_log_do_stop(unsigned int flags)
> > >           memory_region_transaction_begin();
> > >           memory_region_update_pending = true;
> > >           memory_region_transaction_commit();
> > > -        MEMORY_LISTENER_CALL_GLOBAL(log_global_stop, Reverse);
> > > +        MEMORY_LISTENER_CALL_GLOBAL(log_global_stop, Reverse, errp);
> > >       }
> > >   }
> >
> > I'm a little bit surprised to see that MEMORY_LISTENER_CALL_GLOBAL()
> > already allows >2 args, with the ability to conditionally pass over errp
> > with such oneliner change; even if all callers were only using 2 args
> > before this patch..
> yes. The proposal takes the easy path.
>
> Should we change all memory listener global handlers :
>
>   begin
>   commit
>   log_global_after_sync
>   log_global_start
>   log_global_stop
>
> to take an extra Error **errp argument ?
>
> I think we should distinguish begin + commit handlers from the 
> log_global_*
> with a new macro. In which case, we could also change the handler to 
> return
> a bool and fail at the first error in MEMORY_LISTENER_CALL_GLOBAL(...).

I think we must fail at first error in any case. Otherwise, if two 
handlers error and call error_setg() with errp, the second handler will 
assert IIUC.