Hi! On Fri, Jan 27, 2017 at 05:15:29PM +0100, Wim Taymans wrote: > Hi All, > > I took another look at the access control patches. > Thanks a lot for resuming the original work on this; didn't have the time here :-) > There was a desire from Arun and Tanu to have the access control > checks more integrated > into the core objects instead of just some checks in the native protocol. > > I was a bit reluctant to start this because it would involve passing > around a lot of pa_client > objects to many functions in order to do access control. This week, I > put my brain to 0 and > started this anyway, You can find the result in this branch: > > (1) https://cgit.freedesktop.org/~wtay/pulseaudio/log/?h=access-hooks-arg > > I then spent some time thinking about using TLS to pass the client > around, more like a context > to evaluate the functions in than an argument of the function. Tanu > then suggested to put the > current_client in the pa_core object where it can be picked up from > anywhere. The trick is then > to set and clear the current_client in the various protocols. The > result can be seen here: > > (2) https://cgit.freedesktop.org/~wtay/pulseaudio/log/?h=access-hooks-core > > You can still find the old way here: > > (3) https://cgit.freedesktop.org/~wtay/pulseaudio/log/?h=access-hooks > > 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. It does however give > you good control over what > you can check. One example is pa_sink_update_rate() which might > suspend and unsuspend > the source to implement the rate change. It a new client does not have > permission to suspend a > sink, it will not be able to change the rate. > > (2) looks quite ok. Setting and clearing the current_client is > straightforward and easily verifiable. > You can also be certain that when you add new checks, you can just get > the client from there > instead of having to change a method. The downside is that if ever a > non-main thread tries to > do some access control, you get random behaviour. using TLS will, of > course fix this. > > (3) is probably still the most simple solution but only deals with the > native-protocol. (1) and (2) > now also automatically work for the cli, DBus, esound, http, simple > and native protocol. > > What do you think? Which option do you like best? IMHO option (2) sounds like the most generic and forward-looking. Pretty nice to access the client object from everywhere -- similar to the kernel's code pattern where any context can access the current (per-CPU) process context through 'current'. Since PA is mostly eventloop-driven, except in the RT I/O threads, maybe the threading issue is not a big blocker for that choice? After some thinking, I had some worries about any code using pa_threaded_mainloop. Turns out it's mostly used to maintain a synchronous feeling for clients; and in module-zeroconf-public to deal with libavahi-client synchronous calls away from the main eventloop. Unless I'm misunderstanding something, we can also have both, a minimal TLS boolean flag and pa_core_get_current_client()? [*] The current-client accessor would just complain loudly if it discovers that it's called away from the main thread (TLS boolean flag is false). regards, [*] https://cgit.freedesktop.org/~wtay/pulseaudio/commit/?h=access-hooks-core&id=35fd11d9c85042ceb26c73f72c39b09e9bf47a2e -- Darwish http://darwish.chasingpointers.com
