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. -- Tanu https://www.patreon.com/tanuk
