[pulseaudio-discuss] [PATCH] Don't abort on double module load

[Tanu Kaskinen]

On Sat, 2017-08-26 at 11:39 +0200, Colin Leroy wrote:
> Hello,
> 
> I recently had to investigate quite a bit why PulseAudio started to
> abort on my computer, and finally figured out the reason :
> 
> - I had "module-load module-raop-discover" set in /etc/pulse/default.pa
> - I fiddled with `paprefs` while PA was running and checked "Make
>   discoverable Apple AirTunes sound devices available locally"
> - This triggered a load of module-raop-discover via module-gconf, which
>   failed silently because the module was already loaded:
> 
> D: [pulseaudio] module-gconf.c: Loading module 'module-raop-discover'
>   with args '' due to GConf configuration.
> E: [pulseaudio] module.c: Module "module-raop-discover" should be
>   loaded once at most. Refusing to load.
> E: [pulseaudio] module-gconf.c: pa_module_load() failed
> 
> - At the next reboot, module-raop-discover was loaded via module-gconf,
>   then cli-command tried to load it again, failed and PulseAudio exited.
> 
> The attached patch fixes it by adding an optional error code pointer to
> pa_module_load(), and checking for the error code in cli-command.
> 
> I don't know if this is an acceptable solution for the PulseAudio team,
> I'm not even sure that's a case you want to handle, but I thought it
> would be worth it to mention this problem to you, gather your input
> on the subject and come up with an idea to handle it.

The solution seems good to me. Just a couple of minor issues:

We don't have a whole lot of functions that need to both allocate a new
object and report the reason on failure, but there is some precedent.
At least pa_sink_input_new() and pa_source_output_new() are this kind
of functions, and they return the two things differently than
pa_module_load() in your patch. I think we should be consistent, so
pa_module_load() should return the error code as the function return
value, and the first argument of pa_module_load() should be a pointer
to a pa_module pointer.

The error codes should be from the pa_error_code_t enumeration.

pa_module_load() starts with this:

    errcode = -EIO; /* default error */

However, you always set errcode explicitly in every error case (which
is good), so this line is not needed, and it will also prevent the
compiler from issuing a warning if setting errcode is forgotten when
adding a new error case.

-- 
Tanu

https://www.patreon.com/tanuk