On Thu, Jan 30, 2025 at 11:08:40AM +0100, Maciej S. Szmigiero wrote:
> +static int multifd_device_state_save_thread(void *opaque)
> +{
> + struct MultiFDDSSaveThreadData *data = opaque;
> + int ret;
> +
> + ret = data->hdlr(data->idstr, data->instance_id, &send_threads_abort,
> + data->handler_opaque);
I thought we discussed somewhere and the plan was we could use Error** here
to report errors. Would that still make sense, or maybe I lost some
context?
Meanwhile, I still feel uneasy on having these globals (send_threads_abort,
send_threads_ret). Can we make MultiFDDSSaveThreadData the only interface
between migration and the threads impl? So I wonder if it can be:
ret = data->hdlr(data);
With extended struct like this (I added thread_error and thread_quit):
struct MultiFDDSSaveThreadData {
SaveLiveCompletePrecopyThreadHandler hdlr;
char *idstr;
uint32_t instance_id;
void *handler_opaque;
/*
* Should be NULL when struct passed over to thread, the thread should
* set this if the handler would return false. It must be kept NULL if
* the handler returned true / success.
*/
Error *thread_error;
/*
* Migration core would set this when it wants to notify thread to
* quit, for example, when error occured in other threads, or migration is
* cancelled by the user.
*/
bool thread_quit;
};
Then if any multifd_device_state_save_thread() failed, for example, it
should notify all threads to quit by setting thread_quit, instead of
relying on yet another global variable to show migration needs to quit.
Thanks,
> + if (ret && !qatomic_read(&send_threads_ret)) {
> + /*
> + * Racy with the above read but that's okay - which thread error
> + * return we report is purely arbitrary anyway.
> + */
> + qatomic_set(&send_threads_ret, ret);
> + }
> +
> + return 0;
> +}
--
Peter Xu
