On Samstag, 22. August 2020 02:16:23 CEST Geoffrey McRae wrote:
> On 2020-08-22 03:47, Paolo Bonzini wrote:
> > On 21/08/20 19:34, Christian Schoenebeck wrote:
> >>> static void qjack_fini_out(HWVoiceOut *hw)
> >>> {
> >>>
> >>> QJackOut *jo = (QJackOut *)hw;
> >>> qjack_client_fini(&jo->c);
> >>>
> >>> +
> >>> + qemu_bh_delete(jo->c.shutdown_bh);
> >>
> >> Paolo wrapped that qemu_bh_delete() call inside the lock as well. So I
> >> guess
> >> it makes a difference for the BH API?
> >
> > It is not a problem as long as qjack_client_fini is idempotent.
>
> `qjack_client_fini` is indeed idempotent
Right.
> >>> + qemu_mutex_destroy(&jo->c.shutdown_lock);
> >>>
> >>> }
> >>
> >> Hmmm, is this qemu_mutex_destroy() safe at this point?
> >
> > Perhaps make the mutex global and not destroy it at all.
>
> It's safe at this point as `qjack_fini_out` is only called at device
> destruction, and `qjack_client_fini` ensures that JACK is shut down
> which prevents jack from trying to call the shutdown event handler.
You mean because jack_client_close() is synchronized. That prevents JACK from
firing the callback after jack_client_close() returns, that's correct.
But as qemu_bh_delete() is async, you do not have a guarantee that a
previously scheduled BH shutdown handler is no longer running. So it might
still hold the lock when you attempt to destroy the mutex.
On doubt I would do like Paolo suggested by making the mutex global and not
destroying it at all.
Best regards,
Christian Schoenebeck
