Created attachment 53298 [details] [review] Hurd fixes for PulseAudio 1.1 PulseAudio 1.1 does not compile on GNU/Hurd yet. Below there is a list of the various problems (which could be solved in PA upstream) and eventual solutions found: * src/pulsecore/mutex-posix.c: runtime assert() failing pa_mutex_new asserts on setting the PRIO_INHERIT on the mutex attributes, but this is not implemented on Hurd yet; although, given few lines below the code gracefully handles such case, then make that pthread_mutexattr_setprotocol() do that as well * src/modules/rtp/rtp.c: SO_TIMESTAMP most probably the right type to check should be SCM_TIMESTAMP, like other types available for cmsg_type (eg SCM_RIGHTS) * src/modules/rtp/module-rtp-recv.c: SO_TIMESTAMP Hurd does not have this non-POSIX feature yet, so directly fail if SO_TIMESTAMP is not provided in the current platform * src/modules/module-pipe-source.c: PIPE_BUF make use of the available pa_pipe_buf() for the job, hopefully with the correct fd * src/utils/pacmd.c: PIPE_BUF here the two 'ibuf' and 'obuf' are dynamically allocated with the minimum size of all the pipe_buf for the fd's they are used as buffer when reading from and writing to Maybe not all the changes are clean or wanted in PA upstream, but having a review for them would be nice anyway (so we can keep them in Debian GNU/Hurd). (This has been originally reported as http://pulseaudio.org/ticket/817, but that got no feedback in 18 months...)
(In reply to comment #0) > Created attachment 53298 [details] [review] [review] > Hurd fixes for PulseAudio 1.1 > > PulseAudio 1.1 does not compile on GNU/Hurd yet. Below there is a list of the > various problems (which could be solved in PA upstream) and eventual solutions > found: > > * src/pulsecore/mutex-posix.c: runtime assert() failing > pa_mutex_new asserts on setting the PRIO_INHERIT on the mutex attributes, but > this is not implemented on Hurd yet; although, given few lines below the code > gracefully handles such case, then make that pthread_mutexattr_setprotocol() do > that as well Makes sense. To me it would seem more logical to handle setprotocol() gracefully and have a strict assertion for mutex_init() than vice versa like it's done now, but I think failing gracefully in both cases is fine. > * src/modules/rtp/rtp.c: SO_TIMESTAMP > most probably the right type to check should be SCM_TIMESTAMP, like other types > available for cmsg_type (eg SCM_RIGHTS) I couldn't find any real documentation that would have listed either SCM_TIMESTAMP or SO_TIMESTAMP as a valid type - some socket manual pages list SCM_RIGHTS and SCM_UCRED, but usually the possible values aren't explicitly listed at all. This change is probably safe, though, because at least on my Linux machine SCM_TIMESTAMP is defined as SO_TIMESTAMP anyway. > * src/modules/rtp/module-rtp-recv.c: SO_TIMESTAMP > Hurd does not have this non-POSIX feature yet, so directly fail if SO_TIMESTAMP > is not provided in the current platform Ok. > * src/modules/module-pipe-source.c: PIPE_BUF > make use of the available pa_pipe_buf() for the job, hopefully with the correct > fd Most likely correct. There's equivalent code in module-pipe-sink.c. > * src/utils/pacmd.c: PIPE_BUF > here the two 'ibuf' and 'obuf' are dynamically allocated with the minimum size > of all the pipe_buf for the fd's they are used as buffer when reading from and > writing to Looks good. > Maybe not all the changes are clean or wanted in PA upstream, but having a > review for them would be nice anyway (so we can keep them in Debian GNU/Hurd). If I had commit rights, I'd take all these changes in. > (This has been originally reported as http://pulseaudio.org/ticket/817, but > that got no feedback in 18 months...) Sorry to hear that. Sometimes patches get forgotten - it's fine to ping us every now and then if you don't get a timely reply.
As Tanu says, the changes look good. Would you be able to break them up into individual commits (one per actual issue fixed)? Either a tree to pull from, attachments to this bug, or git send-email to our mailing list -- whatever works for you.
Created attachment 53505 [details] [review] Fix for src/modules/module-pipe-source.c
Created attachment 53506 [details] [review] Fix for src/modules/rtp/rtp.c
Created attachment 53507 [details] [review] Fix for src/modules/rtp/module-rtp-recv.c
Created attachment 53508 [details] [review] Fix for src/pulsecore/mutex-posix.c
Created attachment 53509 [details] [review] Fix for src/utils/pacmd.c
Created attachment 53510 [details] [review] Fix for src/modules/rtp/rtp.c
Hi, thanks for the reviews! I've broken up the patch in commits for each issue, and attached them here.
Comment on attachment 53508 [details] [review] Fix for src/pulsecore/mutex-posix.c Review of attachment 53508 [details] [review]: ----------------------------------------------------------------- ::: src/pulsecore/mutex-posix.c @@ +51,5 @@ > > #ifdef HAVE_PTHREAD_PRIO_INHERIT > + if (inherit_priority) { > + r = pthread_mutexattr_setprotocol(&attr, PTHREAD_PRIO_INHERIT); > + pa_assert_se(r == 0 || r == ENOTSUP); This can now become a pa_assert() since there is no side effect. (easily fixed while merging, though, so don't need to reroll this patch)
Sorry about the delay -- pushed all these patches now, thanks!
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.