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

["Cédric Le Goater" <clg@redhat.com>]

On 2/12/24 09:43, Avihai Horon wrote:
> 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.

Good point. I will respin with a new MEMORY_LISTENER_CALL_GLOBAL_ERR macro
exiting the loop at first error.

Thanks,

C.