On Samstag, 7. November 2020 10:58:10 CET Christian Schoenebeck wrote:
> On Samstag, 7. November 2020 01:04:58 CET Geoffrey McRae wrote:
> > This change registers a bottom handler to close the JACK client
> > connection when a server shutdown signal is recieved. Without this
One last minor thing, typo here: "received".
> > libjack2 attempts to "clean up" old clients and causes a use after free
> > segfault.
> >
> > Signed-off-by: Geoffrey McRae <ge...@hostfission.com>
> > ---
> >
> > audio/jackaudio.c | 50 +++++++++++++++++++++++++++++++++++------------
> > 1 file changed, 37 insertions(+), 13 deletions(-)
> >
> > diff --git a/audio/jackaudio.c b/audio/jackaudio.c
> > index 1e714b30bc..e00e19061a 100644
> > --- a/audio/jackaudio.c
> > +++ b/audio/jackaudio.c
> > @@ -25,6 +25,7 @@
> >
> > #include "qemu/osdep.h"
> > #include "qemu/module.h"
> > #include "qemu/atomic.h"
> >
> > +#include "qemu/main-loop.h"
> >
> > #include "qemu-common.h"
> > #include "audio.h"
> >
> > @@ -63,6 +64,7 @@ typedef struct QJackClient {
> >
> > QJackState state;
> > jack_client_t *client;
> > jack_nframes_t freq;
> >
> > + QEMUBH *shutdown_bh;
> >
> > struct QJack *j;
> > int nchannels;
> >
> > @@ -87,6 +89,7 @@ QJackIn;
> >
> > static int qjack_client_init(QJackClient *c);
> > static void qjack_client_connect_ports(QJackClient *c);
> > static void qjack_client_fini(QJackClient *c);
> >
> > +QemuMutex qjack_shutdown_lock;
>
> I think this should be:
>
> static QemuMutex qjack_shutdown_lock;
>
> as this mutex is only accessed from within this unit. Except of that:
>
> Reviewed-by: Christian Schoenebeck <qemu_...@crudebyte.com>
>
> BTW it is common practice to add local functions for initializing,
> destroying, locking and unlocking a specific mutex use case to avoid issues
> when code evolves. But so far the use of this mutex is trivial, so it is Ok
> for now from my PoV.
>
> > static void qjack_buffer_create(QJackBuffer *buffer, int channels, int
> >
> > frames) {
> > @@ -306,21 +309,27 @@ static int qjack_xrun(void *arg)
> >
> > return 0;
> >
> > }
> >
> > +static void qjack_shutdown_bh(void *opaque)
> > +{
> > + QJackClient *c = (QJackClient *)opaque;
> > + qjack_client_fini(c);
> > +}
> > +
> >
> > static void qjack_shutdown(void *arg)
> > {
> >
> > QJackClient *c = (QJackClient *)arg;
> > c->state = QJACK_STATE_SHUTDOWN;
> >
> > + qemu_bh_schedule(c->shutdown_bh);
> >
> > }
> >
> > static void qjack_client_recover(QJackClient *c)
> > {
> >
> > - if (c->state == QJACK_STATE_SHUTDOWN) {
> > - qjack_client_fini(c);
> > + if (c->state != QJACK_STATE_DISCONNECTED) {
> > + return;
> >
> > }
> >
> > /* packets is used simply to throttle this */
> >
> > - if (c->state == QJACK_STATE_DISCONNECTED &&
> > - c->packets % 100 == 0) {
> > + if (c->packets % 100 == 0) {
> >
> > /* if enabled then attempt to recover */
> > if (c->enabled) {
> >
> > @@ -489,15 +498,16 @@ static int qjack_init_out(HWVoiceOut *hw, struct
> > audsettings *as, QJackOut *jo = (QJackOut *)hw;
> >
> > Audiodev *dev = (Audiodev *)drv_opaque;
> >
> > - qjack_client_fini(&jo->c);
> > -
> >
> > jo->c.out = true;
> > jo->c.enabled = false;
> > jo->c.nchannels = as->nchannels;
> > jo->c.opt = dev->u.jack.out;
> >
> > + jo->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, &jo->c);
> > +
> >
> > int ret = qjack_client_init(&jo->c);
> > if (ret != 0) {
> >
> > + qemu_bh_delete(jo->c.shutdown_bh);
> >
> > return ret;
> >
> > }
> >
> > @@ -525,15 +535,16 @@ static int qjack_init_in(HWVoiceIn *hw, struct
> > audsettings *as, QJackIn *ji = (QJackIn *)hw;
> >
> > Audiodev *dev = (Audiodev *)drv_opaque;
> >
> > - qjack_client_fini(&ji->c);
> > -
> >
> > ji->c.out = false;
> > ji->c.enabled = false;
> > ji->c.nchannels = as->nchannels;
> > ji->c.opt = dev->u.jack.in;
> >
> > + ji->c.shutdown_bh = qemu_bh_new(qjack_shutdown_bh, &ji->c);
> > +
> >
> > int ret = qjack_client_init(&ji->c);
> > if (ret != 0) {
> >
> > + qemu_bh_delete(ji->c.shutdown_bh);
> >
> > return ret;
> >
> > }
> >
> > @@ -555,7 +566,7 @@ static int qjack_init_in(HWVoiceIn *hw, struct
> > audsettings *as, return 0;
> >
> > }
> >
> > -static void qjack_client_fini(QJackClient *c)
> > +static void qjack_client_fini_locked(QJackClient *c)
> >
> > {
> >
> > switch (c->state) {
> >
> > case QJACK_STATE_RUNNING:
> > @@ -564,28 +575,40 @@ static void qjack_client_fini(QJackClient *c)
> >
> > case QJACK_STATE_SHUTDOWN:
> > jack_client_close(c->client);
> >
> > + c->client = NULL;
> > +
> > + qjack_buffer_free(&c->fifo);
> > + g_free(c->port);
> > +
> > + c->state = QJACK_STATE_DISCONNECTED;
> >
> > /* fallthrough */
> >
> > case QJACK_STATE_DISCONNECTED:
> > break;
> >
> > }
> >
> > +}
> >
> > - qjack_buffer_free(&c->fifo);
> > - g_free(c->port);
> > -
> > - c->state = QJACK_STATE_DISCONNECTED;
> > +static void qjack_client_fini(QJackClient *c)
> > +{
> > + qemu_mutex_lock(&qjack_shutdown_lock);
> > + qjack_client_fini_locked(c);
> > + qemu_mutex_unlock(&qjack_shutdown_lock);
> >
> > }
> >
> > static void qjack_fini_out(HWVoiceOut *hw)
> > {
> >
> > QJackOut *jo = (QJackOut *)hw;
> > qjack_client_fini(&jo->c);
> >
> > +
> > + qemu_bh_delete(jo->c.shutdown_bh);
> >
> > }
> >
> > static void qjack_fini_in(HWVoiceIn *hw)
> > {
> >
> > QJackIn *ji = (QJackIn *)hw;
> > qjack_client_fini(&ji->c);
> >
> > +
> > + qemu_bh_delete(ji->c.shutdown_bh);
> >
> > }
> >
> > static void qjack_enable_out(HWVoiceOut *hw, bool enable)
> >
> > @@ -662,6 +685,7 @@ static void qjack_info(const char *msg)
> >
> > static void register_audio_jack(void)
> > {
> >
> > + qemu_mutex_init(&qjack_shutdown_lock);
> >
> > audio_driver_register(&jack_driver);
> > jack_set_thread_creator(qjack_thread_creator);
> > jack_set_error_function(qjack_error);
>
> Best regards,
> Christian Schoenebeck
