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

Conversation

amiartus
Copy link

Hello,

please consider accepting following linux specific back-ports developed by ADIT audio team. The patches allow proper responsiveness to some failure states on linux and fix the hang ups that would occur otherwise.

Best Regards,
Adam Miartus

This would be required so that the polling concept can be used to wait
on signals as well as wait for events from the other threads.

(cherry picked from adit)

Change-Id: Ic7b8d4c816d601d4b5467ec9c2519f3c547ae59f
Signed-off-by: Laxmi Devi <[email protected]>
Signed-off-by: Timo Wischer <[email protected]>
… event

On failure, currently the jack thread exits without notifying the jackd.
So jackd is just waiting for the signals, unaware of the failure.
With this implementation, on error threads can post an event to jackd
notifying it to to exit.

(cherry picked from adit commit c430be1e337f52aba8312f7d29990e190cad204a)

Change-Id: Ib484bb1e126f6bff0db76d44e6e036f92f5d8806
Signed-off-by: Laxmi Devi <[email protected]>
Signed-off-by: Timo Wischer <[email protected]>
@nedko
Copy link
Contributor

nedko commented Jul 25, 2023

These look good to me. Thank you! The added signal setup should really be moved to jackctl_setup_signals() in a followup commit (these ones are cherry picks and it is good to keep them like this instead of rewriting git history). For jackdbus at least, it would be good to have epoll() call on the side of the control API user/caller (jackd, jackdbus). Extending the Control API with notion of either libevent (should work on Windows too), or similar abstraction, will achieve this goal.

@nedko
Copy link
Contributor

nedko commented Jul 25, 2023

I added jackctl_finish_signals() to the API: LADI/jack2@ae7b5d0

It is a function to call for cleaning,
after waiting on a signal set,
to undo the jackctl_setup_signals() and restore to previous state.

@amiartus
Copy link
Author

thank you for reviewing and refactoring. looks better to me as it is now after your changes.

Copy link
Contributor

@imaami imaami left a comment

Choose a reason for hiding this comment

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

Some observations. I don't really know JACK-specific details, please double-check my conclusions with that in mind.


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...

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)?


#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.

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.

#ifdef __linux__
fail:
for(int i = 0; i < JackFDCount; i++) {
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.

@amiartus
Copy link
Author

thanks @imaami I can have a look at this over the holidays

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants