Obviously, this question reveals my total ignorance of pulseaudio architecture, but why are you implementing access control in pulseaudio itself, rather than using a firewall wrapper that parses the info being sent down the pulse audio socket and only lets allowed RPC calls through? On 02/15/2017 09:53 AM, Tanu Kaskinen wrote: > On Tue, 2017-02-14 at 13:28 +0100, Wim Taymans wrote: >> On 13 February 2017 at 13:58, Tanu Kaskinen <tanuk at iki.fi> wrote: >>> On Fri, 2017-01-27 at 17:15 +0100, Wim Taymans wrote: >>>> Hi All, >>>> >>>> I think (1) is quite ugly and requires you to modify many functions >>>> with a new parameter that >>>> is not at all related to what the function does. >>> I disagree with the characterization that the client parameter is not >>> related to what the function does. If the function does access control, >>> then the client parameter is very much related to that. >> Together with things such as logging,configuration and allocation, security >> is typically a Cross-cutting concern >> (https://en.wikipedia.org/wiki/Cross-cutting_concern). >> The methods that operate on objects should not include the parameters for >> the cross-cutting concern, you would typically keep those stored in a global >> environment or singleton (like how we have the logging in a singleton, the >> mempool and configuration in pa_core, ..). >> We should to the same with the current client, IMHO. > The existing stuff in pa_core can't be removed and replaced with > function arguments, as far as I can tell. The current_client field can. > > I think having current_client would be completely fine, though, if it > really was only used to store the client whose request is currently > being processed. However, in IRC we talked about needing to set > current_client before calling pa_core_get_sinks() when rescuing > streams, for example. That's quite clearly using a global variable for > just passing a function argument. (This particular problem could be > side-stepped by adding the client argument to pa_core_get_sinks() and > the other similar functions, but otherwise keep using current_client. > As I see it, pa_core_get_sinks() just provides a client-specific view > of the sink list, and it could as well be implemented by storing the > filtered sink list in pa_client, but maintaining that might be more > work than creating the filtered list on demand.) > > Aren't there also going to be problems when one operation involves > another operation, if the first operation is allowed for the client but > second operation isn't? The first operation will fail even though it > was supposed to be allowed. For example, module-bluetooth-policy will > automatically switch the card profile from A2DP to HSP when a source > output appears with media.role=phone. I think that shouldn't be > prevented just because the client that created the source output > doesn't have the right to switch card profiles by itself. I guess > you'll end up setting current_client in module-bluetooth-policy to NULL > before the profile switch while the client still has its stream > creation operation ongoing. (I now realize this wasn't a perfect > example of the problem I first stated, because the source output > creation doesn't fail completely. Maybe a better example would have > been a client that tries to set its stream volume, but it fails because > flat volumes are enabled and the client doesn't have the permission to > set sink volumes.) > > Another example is loading a module: the client's permissions affect > what the module can do during the initialization, which from some > perspective may make sense, but in any case that causes > inconsistencies, because the client's access limitations stop affecting > the module as soon as the initialization is completed. > > But if you think the current_client approach is the best one, I don't > want to override your preference. >
