Re: [PATCH for-9.1 v5 09/14] memory: Add Error** argument to .log_global_start() handler

[Peter Xu <peterx@xxxxxxxxxx>]

On Wed, Mar 20, 2024 at 07:49:05AM +0100, Cédric Le Goater wrote:
> Modify all .log_global_start() handlers to take an Error** parameter
> and return a bool. Adapt memory_global_dirty_log_start() to interrupt
> on the first error the loop on handlers. In such case, a rollback is
> performed to stop dirty logging on all listeners where it was
> previously enabled.
> 
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Anthony Perard <anthony.perard@xxxxxxxxxx>
> Cc: Paul Durrant <paul@xxxxxxx>
> Cc: "Michael S. Tsirkin" <mst@xxxxxxxxxx>
> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Cc: David Hildenbrand <david@xxxxxxxxxx>
> Signed-off-by: Cédric Le Goater <clg@xxxxxxxxxx>

Reviewed-by: Peter Xu <peterx@xxxxxxxxxx>

Still one comment below:

> @@ -3014,8 +3044,11 @@ static void listener_add_address_space(MemoryListener 
> *listener,
>          listener->begin(listener);
>      }
>      if (global_dirty_tracking) {
> +        /*
> +         * Migration has already started. Assert on any error.

If you won't mind, I can change this to:

  /*
   * Currently only VFIO can fail log_global_start(), and it's not allowed
   * to hotplug a VFIO device during migration, so this should never fail
   * when invoked.  If it can start to fail in the future, we need to be
   * able to fail the whole listener_add_address_space() and its callers.
   */

Thanks,

> +         */
>          if (listener->log_global_start) {
> -            listener->log_global_start(listener);
> +            listener->log_global_start(listener, &error_abort);
>          }
>      }
>  
> -- 
> 2.44.0
> 

-- 
Peter Xu