Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adit control api backports #948

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion common/JackAudioDriver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
#include "JackLockedEngine.h"
#include "JackException.h"
#include <assert.h>
#include "JackServerGlobals.h"

using namespace std;

Expand Down Expand Up @@ -199,7 +200,11 @@ int JackAudioDriver::Write()

int JackAudioDriver::Process()
{
return (fEngineControl->fSyncMode) ? ProcessSync() : ProcessAsync();
int err = (fEngineControl->fSyncMode) ? ProcessSync() : ProcessAsync();
if(err && JackServerGlobals::on_failure != NULL) {
JackServerGlobals::on_failure();
}
return err;
}

/*
Expand Down
104 changes: 102 additions & 2 deletions common/JackControlAPI.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@
#include <pthread.h>
#endif

#ifdef __linux__
#include <poll.h>
#include <sys/signalfd.h>
#include <sys/eventfd.h>
#endif

#include "types.h"
#include <string.h>
#include <errno.h>
Expand Down Expand Up @@ -142,6 +148,35 @@ struct jackctl_parameter
jack_driver_param_constraint_desc_t * constraint_ptr;
};

#ifdef __linux__
/** Jack file descriptors */
typedef enum
{
JackSignalFD, /**< @brief File descriptor to accept the signals */
JackEventFD, /**< @brief File descriptor to accept the events from threads */
JackFDCount /**< @brief FD count, ensure this is the last element */
} jackctl_fd;

static int eventFD;
#endif

static
void
on_failure()
{
#ifdef __linux__
int ret = 0;
const uint64_t ev = 1;

ret = write(eventFD, &ev, sizeof(ev));
if (ret < 0) {
fprintf(stderr, "JackServerGlobals::on_failure : write() failed with errno %d\n", -errno);
}
#else
fprintf(stderr, "JackServerGlobals::on_failure callback called from thread\n");
#endif
}

const char * jack_get_self_connect_mode_description(char mode)
{
struct jack_constraint_enum_char_descriptor * descr_ptr;
Expand Down Expand Up @@ -679,16 +714,70 @@ jackctl_setup_signals(
SERVER_EXPORT void
jackctl_wait_signals(jackctl_sigmask_t * sigmask)
{
int sig;
int sig = 0;
bool waiting = true;
#ifdef __linux__
int err;
struct pollfd pfd[JackFDCount];
struct signalfd_siginfo si;
memset(pfd, 0, sizeof(pfd));
Copy link
Contributor

@imaami imaami Sep 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each struct pollfd::fd should be initialized to -1, not 0.


/* Block the signals in order for signalfd to receive them */
sigprocmask(SIG_BLOCK, &sigmask->signals, NULL);

pfd[JackSignalFD].fd = signalfd(-1, &sigmask->signals, 0);
if(pfd[JackSignalFD].fd == -1) {
fprintf(stderr, "signalfd() failed with errno %d\n", -errno);
return;
}
pfd[JackSignalFD].events = POLLIN;

pfd[JackEventFD].fd = eventfd(0, EFD_NONBLOCK);
if(pfd[JackEventFD].fd == -1) {
goto fail;
}
eventFD = pfd[JackEventFD].fd;
pfd[JackEventFD].events = POLLIN;

#endif

while (waiting) {
#if defined(sun) && !defined(__sun__) // SUN compiler only, to check
sigwait(&sigmask->signals);
fprintf(stderr, "Jack main caught signal\n");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should a library API function such as this really print errors directly to stderr like this? I'm not asking this rhetorically, I genuinely don't know what the expectation is regarding jackctl API behavior. I wanted to bring this up just in case it matters.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure either, but logging is not consistent all over jack2, some functions use printf some jack_log/jack_error. Its not clear to me when to use what...

#elif defined(__linux__)
err = poll(pfd, JackFDCount, -1);
if (err < 0) {
if (errno == EINTR) {
continue;
} else {
fprintf(stderr, "Jack : poll() failed with errno %d\n", -errno);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless of how error reporting should work here, printing the negation of errno seems bizarre. AFAIK only kernel code uses negative errno values due to implementation reasons, but in this situation I can't see how it's anything but a magic number that also happens to be even less useful than an unmodified errno. If there must be stderr output, why not just call strerror(errno)?

break;
}
} else {
if ((pfd[JackSignalFD].revents & (POLLERR | POLLHUP | POLLNVAL)) ||
pfd[JackEventFD].revents & (POLLERR | POLLHUP | POLLNVAL)) {
fprintf(stderr, "Jack : poll() exited with errno %d\n", -errno);
break;
} else if ((pfd[JackSignalFD].revents & POLLIN) != 0) {
err = read (pfd[JackSignalFD].fd, &si, sizeof(si));
if (err < 0) {
fprintf(stderr, "Jack : read() on signalfd failed with errno %d\n", -errno);
break;
}
sig = si.ssi_signo;
fprintf(stderr, "Jack main caught signal %d\n", sig);
} else if ((pfd[JackEventFD].revents & POLLIN) != 0) {
sig = 0; /* Received an event from one of the Jack thread */
fprintf(stderr, "Jack main received event from child thread, Exiting\n");
} else {
continue;
}
}
#else
sigwait(&sigmask->signals, &sig);
#endif
fprintf(stderr, "Jack main caught signal %d\n", sig);
#endif

switch (sig) {
case SIGUSR1:
Expand All @@ -712,6 +801,16 @@ jackctl_wait_signals(jackctl_sigmask_t * sigmask)
// bugs that cause segfaults etc. during shutdown.
sigprocmask(SIG_UNBLOCK, &sigmask->signals, 0);
}

#ifdef __linux__
fail:
for(int i = 0; i < JackFDCount; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and a handful of other lines don't conform to the style of the rest of the code. AFAIK there should be a space after if, for, etc.

if(pfd[i].fd != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong. If eventfd() fails then close(-1) gets called here. Initialize all fds to -1 and compare against that as the canonical "invalid fd" value.

close(pfd[i].fd);
}
}
#endif

}
#endif

Expand Down Expand Up @@ -931,6 +1030,7 @@ SERVER_EXPORT jackctl_server_t * jackctl_server_create2(
JackServerGlobals::on_device_acquire = on_device_acquire;
JackServerGlobals::on_device_release = on_device_release;
JackServerGlobals::on_device_reservation_loop = on_device_reservation_loop;
JackServerGlobals::on_failure = on_failure;

if (!jackctl_drivers_load(server_ptr))
{
Expand Down
1 change: 1 addition & 0 deletions common/JackServerGlobals.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ std::map<std::string, int> JackServerGlobals::fInternalsList;
bool (* JackServerGlobals::on_device_acquire)(const char * device_name) = NULL;
void (* JackServerGlobals::on_device_release)(const char * device_name) = NULL;
void (* JackServerGlobals::on_device_reservation_loop)(void) = NULL;
void (* JackServerGlobals::on_failure)() = NULL;

int JackServerGlobals::Start(const char* server_name,
jack_driver_desc_t* driver_desc,
Expand Down
1 change: 1 addition & 0 deletions common/JackServerGlobals.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ struct SERVER_EXPORT JackServerGlobals
static bool (* on_device_acquire)(const char* device_name);
static void (* on_device_release)(const char* device_name);
static void (* on_device_reservation_loop)(void);
static void (* on_failure)(); /* Optional callback to be called from any thread on failure */

JackServerGlobals();
~JackServerGlobals();
Expand Down