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
