Watch qdb entry being set and modified on start or while playing #211
Conversation
|
It might deserves more logging messages but this is a first draft approach. In order to have a smooth switching experience, it requires that (see QubesOS/qubes-gui-daemon#141) |
pipewire/qubes-pw-module.c
Outdated
| do { | ||
| res = spa_loop_invoke(impl->main_loop, add_qdb_cb, 0, NULL, 0, true, impl); | ||
| } while (res == -EPIPE); |
There was a problem hiding this comment.
This code is already running on the main thread. Just call add_qdb_cb directly.
pipewire/qubes-pw-module.c
Outdated
|
|
||
| qdb_watch(impl->qdb, QUBES_AUDIOVM_QUBESDB_ENTRY); | ||
| qdb_fd = qdb_watch_fd(impl->qdb); | ||
| spa_assert_se(qdb_fd != -EPIPE); |
There was a problem hiding this comment.
Whta does -EPIPE mean here?
There was a problem hiding this comment.
There was a problem hiding this comment.
That will cause a core dump. Is that really the intended behavior? I think the code should instead gracefully handle the case where the connection to qubesdb is lost.
pipewire/qubes-pw-module.c
Outdated
| struct impl *impl = user_data; | ||
| int qdb_fd; | ||
|
|
||
| qdb_watch(impl->qdb, QUBES_AUDIOVM_QUBESDB_ENTRY); |
There was a problem hiding this comment.
Please check the return value.
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.2&build=2024042610-4.2&flavor=pull-requests New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.2&build=2024041501-4.2&flavor=update
Failed tests20 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/97064#dependencies 20 fixed
Unstable tests
|
pipewire/qubes-pw-module.c
Outdated
|
|
||
| qdb_watch(impl->qdb, QUBES_AUDIOVM_QUBESDB_ENTRY); | ||
| qdb_fd = qdb_watch_fd(impl->qdb); | ||
| spa_assert_se(qdb_fd != -EPIPE); |
There was a problem hiding this comment.
That will cause a core dump. Is that really the intended behavior? I think the code should instead gracefully handle the case where the connection to qubesdb is lost.
| pw_log_error("Could not open QubesDB"); | ||
| goto error; | ||
| } | ||
| impl->qdb = qdb; |
There was a problem hiding this comment.
Nit: this should be cleaned up when the module is destroyed. Nit because right now that only happens at process exit anyway.
|
Relevant audiovm tests have passed :) |
|
@DemiMarie @marmarek Are we good for this PR? |
|
@fepitre good from my side |
pipewire/qubes-pw-module.c
Outdated
| pw_log_info("AudioVM unset, disconnecting from %d", impl->domid); | ||
| else { | ||
| errno = -domid; | ||
| pw_log_error("Cannot obtain new peer domain ID: %m"); |
There was a problem hiding this comment.
Shouldn't it keep old impl->domid in this case?
There was a problem hiding this comment.
This is going to be either a negative value or an acceptable value. It will change in all the case on new audiovm so I don't think it matters.
There was a problem hiding this comment.
Ok, so you interpret failing to read as "no audiovm". I'm fine with this, but please adjust the message to be clear about it.
pipewire/qubes-pw-module.c
Outdated
| impl->domid = domid; | ||
| for (int i = 0; i < 2; ++i) { | ||
| if (impl->stream[i].source.fd != -1) | ||
| pw_log_error("Closing %d", impl->stream[i].source.fd); |
There was a problem hiding this comment.
I don't think it's an error, more like info?. And also, why only log message is under this "if", is that intentional?
There was a problem hiding this comment.
Because this message is not pertinent if there is no fd, we don't close -1 as this is the value saying there is no fd.
pipewire/qubes-pw-module.c
Outdated
| goto error; | ||
| } | ||
| impl->qdb = qdb; | ||
| impl->domid = get_domid_from_qdb(impl->qdb); |
There was a problem hiding this comment.
Getting domid should be done just after setting up the watch, otherwise you'll miss modification between those two calls.
fepitre
force-pushed
the
devel20240324
branch
2 times, most recently
from
a0aa211 to
4ff04a6
Compare
- Fix missing dependency for ArchLinux Related to QubesOS/qubes-issues#8975
QubesOS/qubes-issues#8975