-
Notifications
You must be signed in to change notification settings - Fork 374
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
base: develop
Are you sure you want to change the base?
Conversation
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 <Laxmi.Devi@in.bosch.com> Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
… 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 <Laxmi.Devi@in.bosch.com> Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>
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. |
I added jackctl_finish_signals() to the API: LADI/jack2@ae7b5d0 It is a function to call for cleaning, |
thank you for reviewing and refactoring. looks better to me as it is now after your changes. |
There was a problem hiding this 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"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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++) { |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
thanks @imaami I can have a look at this over the holidays |
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