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

[Peter Xu]

On Wed, Mar 06, 2024 at 02:34:28PM +0100, Cédric Le Goater wrote:
> diff --git a/system/memory.c b/system/memory.c
> index 
> a229a79988fce2aa3cb77e3a130db4c694e8cd49..3600e716149407c10a1f6bf8f0a81c2611cf15ba
>  100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -2914,9 +2914,27 @@ static unsigned int postponed_stop_flags;
>  static VMChangeStateEntry *vmstate_change;
>  static void memory_global_dirty_log_stop_postponed_run(void);
>  
> +/*
> + * Stop dirty logging on all listeners where it was previously enabled.
> + */
> +static void memory_global_dirty_log_rollback(MemoryListener *listener,
> +                                             unsigned int flags)
> +{
> +    global_dirty_tracking &= ~flags;

Having a hook rollback function to touch the global_dirty_tracking flag is
IMHO tricky.

Can we instead provide a helper to call all log_global_start() hooks, but
allow a gracefully fail (so rollback will be called if it fails)?

  bool memory_global_dirty_log_start_hooks(...)

Or any better names..  Leaving global_dirty_tracking rollback to
memory_global_dirty_log_start() when it returns false.

Would this be cleaner?

> +    trace_global_dirty_changed(global_dirty_tracking);
> +
> +    while (listener) {
> +        if (listener->log_global_stop) {
> +            listener->log_global_stop(listener);
> +        }
> +        listener = QTAILQ_PREV(listener, link);
> +    }
> +}
> +
>  void memory_global_dirty_log_start(unsigned int flags)
>  {
>      unsigned int old_flags;
> +    Error *local_err = NULL;
>  
>      assert(flags && !(flags & (~GLOBAL_DIRTY_MASK)));
>  
> @@ -2936,7 +2954,25 @@ 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);
> +        MemoryListener *listener;
> +        bool ret = true;
> +
> +        QTAILQ_FOREACH(listener, &memory_listeners, link) {
> +            if (listener->log_global_start) {
> +                ret = listener->log_global_start(listener, &local_err);
> +                if (!ret) {
> +                    break;
> +                }
> +            }
> +        }
> +
> +        if (!ret) {
> +            memory_global_dirty_log_rollback(QTAILQ_PREV(listener, link),
> +                                             flags);
> +            error_report_err(local_err);
> +            return;
> +        }
> +
>          memory_region_transaction_begin();
>          memory_region_update_pending = true;
>          memory_region_transaction_commit();
> @@ -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..

Thanks,

>          }
>      }
>  
> -- 
> 2.44.0
> 

-- 
Peter Xu