Bug 42715

Summary: Does not compile on GNU/Hurd
Product: PulseAudio Reporter: Pino Toscano <toscano.pino>
Component: miscAssignee: pulseaudio-bugs
Status: RESOLVED FIXED QA Contact: pulseaudio-bugs
Severity: enhancement    
Priority: medium CC: lennart
Version: unspecified   
Hardware: All   
OS: other   
Whiteboard:
i915 platform: i915 features:
Attachments: Hurd fixes for PulseAudio 1.1
Fix for src/modules/module-pipe-source.c
Fix for src/modules/rtp/rtp.c
Fix for src/modules/rtp/module-rtp-recv.c
Fix for src/pulsecore/mutex-posix.c
Fix for src/utils/pacmd.c
Fix for src/modules/rtp/rtp.c

Description Pino Toscano 2011-11-08 10:10:35 UTC
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...)
Comment 1 Tanu Kaskinen 2011-11-08 12:47:38 UTC
(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.
Comment 2 Arun Raghavan 2011-11-13 20:58:34 UTC
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.
Comment 3 Pino Toscano 2011-11-14 03:24:11 UTC
Created attachment 53505 [details] [review]
Fix for src/modules/module-pipe-source.c
Comment 4 Pino Toscano 2011-11-14 03:24:22 UTC
Created attachment 53506 [details] [review]
Fix for src/modules/rtp/rtp.c
Comment 5 Pino Toscano 2011-11-14 03:24:33 UTC
Created attachment 53507 [details] [review]
Fix for src/modules/rtp/module-rtp-recv.c
Comment 6 Pino Toscano 2011-11-14 03:24:43 UTC
Created attachment 53508 [details] [review]
Fix for src/pulsecore/mutex-posix.c
Comment 7 Pino Toscano 2011-11-14 03:24:54 UTC
Created attachment 53509 [details] [review]
Fix for src/utils/pacmd.c
Comment 8 Pino Toscano 2011-11-14 03:33:37 UTC
Created attachment 53510 [details] [review]
Fix for src/modules/rtp/rtp.c
Comment 9 Pino Toscano 2011-11-14 03:35:59 UTC
Hi,

thanks for the reviews! I've broken up the patch in commits for each issue, and attached them here.
Comment 10 Arun Raghavan 2012-01-01 19:48:50 UTC
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)
Comment 11 Arun Raghavan 2012-01-02 11:08:56 UTC
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.