From 6414cd9dbf2c864f594ab5443f310adb0e46c383 Mon Sep 17 00:00:00 2001 From: Nedko Arnaudov Date: Sat, 24 Nov 2012 04:29:36 +0200 Subject: [PATCH 1/3] Improved error reporting in device reservation code rd_acquire() error handling is adjusted to match libdbus error handling convention: * Initialize error before calling rd_acquire() * Free dbus error on rd_acquire() failures * Always set dbus error on rd_acquire failures, except for detected programming errors that now cause taboo assert. --- dbus/audio_reserve.c | 3 ++ dbus/controller.c | 3 ++ dbus/reserve.c | 80 ++++++++++++++++++++++++++++++++++---------- 3 files changed, 68 insertions(+), 18 deletions(-) diff --git a/dbus/audio_reserve.c b/dbus/audio_reserve.c index da811c83..9784d1b6 100644 --- a/dbus/audio_reserve.c +++ b/dbus/audio_reserve.c @@ -76,6 +76,8 @@ SERVER_EXPORT bool audio_acquire(const char * device_name) assert(gReserveCount < DEVICE_MAX); + dbus_error_init(&error); + if ((ret= rd_acquire( &gReservedDevice[gReserveCount].reserved_device, gConnection, @@ -86,6 +88,7 @@ SERVER_EXPORT bool audio_acquire(const char * device_name) &error)) < 0) { jack_error("Failed to acquire device name : %s error : %s", device_name, (error.message ? error.message : strerror(-ret))); + dbus_error_free(&error); return false; } diff --git a/dbus/controller.c b/dbus/controller.c index cada1f07..f4ed8397 100644 --- a/dbus/controller.c +++ b/dbus/controller.c @@ -377,6 +377,8 @@ on_device_acquire(const char * device_name) int ret; DBusError error; + dbus_error_init(&error); + ret = rd_acquire( &g_reserved_device[g_device_count].reserved_device, g_connection, @@ -388,6 +390,7 @@ on_device_acquire(const char * device_name) if (ret < 0) { jack_error("Failed to acquire device name : %s error : %s", device_name, (error.message ? error.message : strerror(-ret))); + dbus_error_free(&error); return false; } diff --git a/dbus/reserve.c b/dbus/reserve.c index 88e91b2b..cae77e64 100644 --- a/dbus/reserve.c +++ b/dbus/reserve.c @@ -31,6 +31,11 @@ #include #include "reserve.h" +#include "jack/control.h" + +#define RESERVE_ERROR_NO_MEMORY "org.freedesktop.ReserveDevice1.Error.NoMemory" +#define RESERVE_ERROR_PROTOCOL_VIOLATION "org.freedesktop.ReserveDevice1.Error.Protocol" +#define RESERVE_ERROR_RELEASE_DENIED "org.freedesktop.ReserveDevice1.Error.ReleaseDenied" struct rd_device { int ref; @@ -371,34 +376,51 @@ int rd_acquire( dbus_bool_t good; vtable.message_function = object_handler; - if (!error) + if (!error) { error = &_error; + dbus_error_init(error); + } - dbus_error_init(error); - - if (!_d) - return -EINVAL; + if (!_d) { + assert(0); + r = -EINVAL; + goto fail; + } - if (!connection) - return -EINVAL; + if (!connection) { + assert(0); + r = -EINVAL; + goto fail; + } - if (!device_name) - return -EINVAL; + if (!device_name) { + assert(0); + r = -EINVAL; + goto fail; + } - if (!request_cb && priority != INT32_MAX) - return -EINVAL; + if (!request_cb && priority != INT32_MAX) { + assert(0); + r = -EINVAL; + goto fail; + } - if (!(d = (rd_device *)calloc(sizeof(rd_device), 1))) - return -ENOMEM; + if (!(d = (rd_device *)calloc(sizeof(rd_device), 1))) { + dbus_set_error(error, RESERVE_ERROR_NO_MEMORY, "Cannot allocate memory for rd_device struct"); + r = -ENOMEM; + goto fail; + } d->ref = 1; if (!(d->device_name = strdup(device_name))) { + dbus_set_error(error, RESERVE_ERROR_NO_MEMORY, "Cannot duplicate device name string"); r = -ENOMEM; goto fail; } if (!(d->application_name = strdup(application_name))) { + dbus_set_error(error, RESERVE_ERROR_NO_MEMORY, "Cannot duplicate application name string"); r = -ENOMEM; goto fail; } @@ -408,12 +430,14 @@ int rd_acquire( d->request_cb = request_cb; if (!(d->service_name = (char*)malloc(sizeof(SERVICE_PREFIX) + strlen(device_name)))) { + dbus_set_error(error, RESERVE_ERROR_NO_MEMORY, "Cannot allocate memory for service name string"); r = -ENOMEM; goto fail; } sprintf(d->service_name, SERVICE_PREFIX "%s", d->device_name); if (!(d->object_path = (char*)malloc(sizeof(OBJECT_PREFIX) + strlen(device_name)))) { + dbus_set_error(error, RESERVE_ERROR_NO_MEMORY, "Cannot allocate memory for object path string"); r = -ENOMEM; goto fail; } @@ -425,20 +449,28 @@ int rd_acquire( DBUS_NAME_FLAG_DO_NOT_QUEUE| (priority < INT32_MAX ? DBUS_NAME_FLAG_ALLOW_REPLACEMENT : 0), error)) < 0) { + jack_error("dbus_bus_request_name() failed. (1)"); r = -EIO; goto fail; } - if (k == DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER || k == DBUS_REQUEST_NAME_REPLY_ALREADY_OWNER) + switch (k) { + case DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER: + case DBUS_REQUEST_NAME_REPLY_ALREADY_OWNER: goto success; - - if (k != DBUS_REQUEST_NAME_REPLY_EXISTS) { + case DBUS_REQUEST_NAME_REPLY_EXISTS: + break; + case DBUS_REQUEST_NAME_REPLY_IN_QUEUE : /* DBUS_NAME_FLAG_DO_NOT_QUEUE was specified */ + default: /* unknown reply returned */ + jack_error("request name reply with unexpected value %d.", k); + assert(0); r = -EIO; goto fail; } if (priority <= INT32_MIN) { r = -EBUSY; + dbus_set_error(error, RESERVE_ERROR_RELEASE_DENIED, "Device reservation request with priority %"PRIi32" denied for \"%s\"", priority, device_name); goto fail; } @@ -447,6 +479,7 @@ int rd_acquire( d->object_path, "org.freedesktop.ReserveDevice1", "RequestRelease"))) { + dbus_set_error(error, RESERVE_ERROR_NO_MEMORY, "Cannot allocate memory for RequestRelease method call"); r = -ENOMEM; goto fail; } @@ -455,6 +488,7 @@ int rd_acquire( m, DBUS_TYPE_INT32, &d->priority, DBUS_TYPE_INVALID)) { + dbus_set_error(error, RESERVE_ERROR_NO_MEMORY, "Cannot append args for RequestRelease method call"); r = -ENOMEM; goto fail; } @@ -469,10 +503,12 @@ int rd_acquire( dbus_error_has_name(error, DBUS_ERROR_UNKNOWN_METHOD) || dbus_error_has_name(error, DBUS_ERROR_NO_REPLY)) { /* This must be treated as denied. */ + jack_info("Device reservation request with priority %"PRIi32" denied for \"%s\": %s (%s)", priority, device_name, error->name, error->message); r = -EBUSY; goto fail; } + jack_error("dbus_connection_send_with_reply_and_block(RequestRelease) failed."); r = -EIO; goto fail; } @@ -482,11 +518,13 @@ int rd_acquire( error, DBUS_TYPE_BOOLEAN, &good, DBUS_TYPE_INVALID)) { + jack_error("RequestRelease() reply is invalid."); r = -EIO; goto fail; } if (!good) { + dbus_set_error(error, RESERVE_ERROR_RELEASE_DENIED, "Device reservation request with priority %"PRIi32" denied for \"%s\" via RequestRelease()", priority, device_name); r = -EBUSY; goto fail; } @@ -498,11 +536,14 @@ int rd_acquire( (priority < INT32_MAX ? DBUS_NAME_FLAG_ALLOW_REPLACEMENT : 0)| DBUS_NAME_FLAG_REPLACE_EXISTING, error)) < 0) { + jack_error("dbus_bus_request_name() failed. (2)"); r = -EIO; goto fail; } if (k != DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER) { + /* this is racy, another contender may have acquired the device */ + dbus_set_error(error, RESERVE_ERROR_PROTOCOL_VIOLATION, "request name reply is not DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER but %d.", k); r = -EIO; goto fail; } @@ -510,11 +551,13 @@ int rd_acquire( success: d->owning = 1; - if (!(dbus_connection_register_object_path( + if (!(dbus_connection_try_register_object_path( d->connection, d->object_path, &vtable, - d))) { + d, + error))) { + jack_error("cannot register object path \"%s\": %s", d->object_path, error->message); r = -ENOMEM; goto fail; } @@ -526,6 +569,7 @@ success: filter_handler, d, NULL)) { + dbus_set_error(error, RESERVE_ERROR_NO_MEMORY, "Cannot add filter"); r = -ENOMEM; goto fail; } From 6afe197889b141251f7f68d8b76706097c7ebd57 Mon Sep 17 00:00:00 2001 From: Nedko Arnaudov Date: Sat, 24 Nov 2012 04:39:46 +0200 Subject: [PATCH 2/3] Fixes for alsa device reservation * Don't attempt to use device reservation when card_to_num() mapping has failed. In case of card_to_num() failures, "Audio-1" device was reserved on start but not released on stop. This was causing next start to fail with out of memory error, and since previous commit, with "A handler is already registered for /org/freedesktop/ReserveDevice1/Audio-1" error. * In case of playback device reservation failure, release the capture device * Remove unused fReservedCaptureDevice and fReservedPlaybackDevice data members of the JackAlsaDriver class --- linux/alsa/JackAlsaDriver.cpp | 16 +++++++++++----- linux/alsa/JackAlsaDriver.h | 4 +--- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/linux/alsa/JackAlsaDriver.cpp b/linux/alsa/JackAlsaDriver.cpp index a79772f2..2465b551 100644 --- a/linux/alsa/JackAlsaDriver.cpp +++ b/linux/alsa/JackAlsaDriver.cpp @@ -278,16 +278,22 @@ int JackAlsaDriver::Open(jack_nframes_t nframes, int playback_card = card_to_num(playback_driver_name); char audio_name[32]; - snprintf(audio_name, sizeof(audio_name), "Audio%d", capture_card); - if (!JackServerGlobals::on_device_acquire(audio_name)) { - jack_error("Audio device %s cannot be acquired...", capture_driver_name); - return -1; + if (capture_card >= 0) { + snprintf(audio_name, sizeof(audio_name), "Audio%d", capture_card); + if (!JackServerGlobals::on_device_acquire(audio_name)) { + jack_error("Audio device %s cannot be acquired...", capture_driver_name); + return -1; + } } - if (playback_card != capture_card) { + if (playback_card >= 0 && playback_card != capture_card) { snprintf(audio_name, sizeof(audio_name), "Audio%d", playback_card); if (!JackServerGlobals::on_device_acquire(audio_name)) { jack_error("Audio device %s cannot be acquired...", playback_driver_name); + if (capture_card >= 0) { + snprintf(audio_name, sizeof(audio_name), "Audio%d", capture_card); + JackServerGlobals::on_device_release(audio_name); + } return -1; } } diff --git a/linux/alsa/JackAlsaDriver.h b/linux/alsa/JackAlsaDriver.h index a88d4a01..c62ff541 100644 --- a/linux/alsa/JackAlsaDriver.h +++ b/linux/alsa/JackAlsaDriver.h @@ -39,15 +39,13 @@ class JackAlsaDriver : public JackAudioDriver private: jack_driver_t* fDriver; - int fReservedCaptureDevice; - int fReservedPlaybackDevice; void UpdateLatencies(); public: JackAlsaDriver(const char* name, const char* alias, JackLockedEngine* engine, JackSynchro* table) - : JackAudioDriver(name, alias, engine, table),fDriver(NULL),fReservedCaptureDevice(-1),fReservedPlaybackDevice(-1) + : JackAudioDriver(name, alias, engine, table),fDriver(NULL) {} virtual ~JackAlsaDriver() {} From b3394f4dcec1f707403796615d7bb6f0944183a3 Mon Sep 17 00:00:00 2001 From: Nedko Arnaudov Date: Sat, 24 Nov 2012 05:08:09 +0200 Subject: [PATCH 3/3] Improve computation of the alsa control device name We recommend using symbolic names like hw:Live but when subdevices are used regcomp() was failing to parse them. This changeset improves the algorithm by using less assumptions. --- linux/alsa/JackAlsaDriver.cpp | 38 +++++++++++++++++------------------ linux/alsa/alsa_driver.c | 20 +++--------------- 2 files changed, 22 insertions(+), 36 deletions(-) diff --git a/linux/alsa/JackAlsaDriver.cpp b/linux/alsa/JackAlsaDriver.cpp index 2465b551..4c62f338 100644 --- a/linux/alsa/JackAlsaDriver.cpp +++ b/linux/alsa/JackAlsaDriver.cpp @@ -31,7 +31,6 @@ Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. #include #include #include -#include #include #include "JackAlsaDriver.h" @@ -172,30 +171,31 @@ int JackAlsaDriver::Detach() return JackAudioDriver::Detach(); } -static char* get_control_device_name(const char * device_name) +extern "C" char* get_control_device_name(const char * device_name) { char * ctl_name; - regex_t expression; + const char * comma; - regcomp(&expression, "(plug)?hw:[0-9](,[0-9])?", REG_ICASE | REG_EXTENDED); + /* the user wants a hw or plughw device, the ctl name + * should be hw:x where x is the card identification. + * We skip the subdevice suffix that starts with comma */ - if (!regexec(&expression, device_name, 0, NULL, 0)) { - /* the user wants a hw or plughw device, the ctl name - * should be hw:x where x is the card number */ - - char tmp[5]; - strncpy(tmp, strstr(device_name, "hw"), 4); - tmp[4] = '\0'; - jack_info("control device %s",tmp); - ctl_name = strdup(tmp); - } else { - ctl_name = strdup(device_name); + if (strncasecmp(device_name, "plughw:", 7) == 0) { + /* skip the "plug" prefix" */ + device_name += 4; } - regfree(&expression); - - if (ctl_name == NULL) { - jack_error("strdup(\"%s\") failed.", ctl_name); + comma = strchr(device_name, ','); + if (comma == NULL) { + ctl_name = strdup(device_name); + if (ctl_name == NULL) { + jack_error("strdup(\"%s\") failed.", device_name); + } + } else { + ctl_name = strndup(device_name, comma - device_name); + if (ctl_name == NULL) { + jack_error("strndup(\"%s\", %u) failed.", device_name, (unsigned int)(comma - device_name)); + } } return ctl_name; diff --git a/linux/alsa/alsa_driver.c b/linux/alsa/alsa_driver.c index 8e588e93..e14458b6 100644 --- a/linux/alsa/alsa_driver.c +++ b/linux/alsa/alsa_driver.c @@ -32,7 +32,6 @@ #include #include #include -#include #include #include "alsa_driver.h" @@ -137,30 +136,18 @@ alsa_driver_check_capabilities (alsa_driver_t *driver) return 0; } +char* get_control_device_name(const char * device_name); + static int alsa_driver_check_card_type (alsa_driver_t *driver) { int err; snd_ctl_card_info_t *card_info; char * ctl_name; - regex_t expression; snd_ctl_card_info_alloca (&card_info); - regcomp(&expression,"(plug)?hw:[0-9](,[0-9])?",REG_ICASE|REG_EXTENDED); - - if (!regexec(&expression,driver->alsa_name_playback,0,NULL,0)) { - /* the user wants a hw or plughw device, the ctl name - * should be hw:x where x is the card number */ - - char tmp[5]; - strncpy(tmp,strcasestr(driver->alsa_name_playback,"hw"),4); - tmp[4]='\0'; - jack_info("control device %s",tmp); - ctl_name = strdup(tmp); - } else { - ctl_name = strdup(driver->alsa_name_playback); - } + ctl_name = get_control_device_name(driver->alsa_name_playback); // XXX: I don't know the "right" way to do this. Which to use // driver->alsa_name_playback or driver->alsa_name_capture. @@ -175,7 +162,6 @@ alsa_driver_check_card_type (alsa_driver_t *driver) driver->alsa_driver = strdup(snd_ctl_card_info_get_driver (card_info)); - regfree(&expression); free(ctl_name); return alsa_driver_check_capabilities (driver);