Bug 42715

(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!