[pulseaudio-discuss] [PATCH 1/2] protocol-native: Add commands for communication with modules

Fri Jul 7 14:12:36 UTC 2017

On Thu, 2017-07-06 at 21:35 +0200, Georg Chini wrote:
> On 06.07.2017 15:25, Tanu Kaskinen wrote:
> > On Wed, 2017-07-05 at 08:56 +0200, Georg Chini wrote:
> > > On 04.07.2017 18:15, Tanu Kaskinen wrote:
> > > > On Sun, 2017-05-21 at 14:56 +0200, Georg Chini wrote:
> > > > > This patch adds two new commands to the native protocol that enable direct
> > > > > communication with modules during run time. SEND_MODULE_COMMAND is used to
> > > > > send a command string to a module if no reply is needed. This can for example
> > > > > be employed to change parameters on the fly. SEND_MODULE_QUERY is similar to
> > > > > SEND_MODULE_COMMAND but expects a reply string from the module. This command
> > > > > can for example be used to query the current value of parameters.
> > > > > Within the module, the functions set_command_callback() and get_command_callback()
> > > > > need to be implemented to handle the two types of requests.
> > > > > 
> > > > > This is a rather generic interface because it only passes strings between the
> > > > > module and a client without making any assumptions about the content of the
> > > > > strings.
> > > 
> > > I'm not sure I fully understand your reply. The commands I added were
> > > just intended to have a simple way to set/get module parameters but it
> > > looks like you are seeing a lot more potential.
> > > 
> > > > So far we've used "extensions" for module-specific commands. This seems
> > > > to be an alternative solution to the same problem. I would expect some
> > > > explanation in the commit message for why this solution is better than
> > > > the existing one.
> > > 
> > > I did not even think about the extension system when writing the
> > > code. If I should argue why this is better, I would say mainly because
> > > it is not necessary to add native protocol commands. Once a command
> > > handler is available, new commands need only be added there. It also
> > > gives more flexibility because an arbitrary string can be passed to
> > > the command handler.
> > > 
> > > > I'm not necessarily opposed to replacing the extension system with
> > > > something new. The extension system is cumbersome to work with, and
> > > > it's probably possible to come up with something better.
> > > > 
> > > > If I recall correctly, you mentioned (probably in IRC) configuring
> > > > equalizer parameters as one possible use case, and my reply was that
> > > > equalizer instances don't necessarily map directly to module instances,
> > > > so "pa_context_send_module_command()" isn't really appropriate; at
> > > > least the function name needs to be changed. D-Bus uses "objects" as
> > > > message targets, and each object has a unique path string as an
> > > > identifier. I think a similar approach would be good here: messages
> > > > should just carry an arbitrary string as the recipient indentifier, it
> > > > doesn't matter whether the recipient is a module or something else.
> > > 
> > > I see the point. We could let any entity register a command handler.
> > > Any idea about the naming scheme?
> > 
> > My suggestion:
> > 
> > pa_context_send_message(context, recipient, message_name, message_params, cb, userdata);
> 
> Looks good but does not really answer my intended question. Let me
> ask differently: How should the recipients be named?

If the recipient is an object as I originally intended, then the object
name could be used (e.g. sink name). If the object doesn't currently
have a name (e.g. sink inputs), then I think a name should be added. I
think it would be good to have names on all objects, as long as they
are more informative than just the object index.

If the recipient identifies just the implementor of a collection of
functions like you suggest, then the name should be something suitable
for that. For example, the loopback API could be named "loopback". I
don't want to use "module-loopback" as the name, because the API may
not be forever provided by a module called that. Loopbacks could very
well end up being included in the core, or loopbacks could be provided
by a library that is used by other modules to create loopbacks without
needing to load any module-loopback instances. Loading and unloading
modules and then controlling those modules from policy modules is more
cumbersome than using a direct C API like pa_loopback_new() and
pa_loopback_free() and pa_loopback_set_foo().

> > I now noticed that in your patch the message name and parameters are
> > not separate. I think it's good to be able to register separate
> > handlers for separate messages. Multiple modules might want to register
> > different message handlers for a single recipient object. For example,
> > the alsa modules could register alsa specific sink messages and an
> > equalizer module could register equalizer messages for the same sink.
> 
> You lost me here. For me there is one handler per recipient. In your
> example above, the recipients would be the modules, even though
> they work on the same sink. And if a module uses some kind of plug-in,
> the plug-in could also register its own handler. A module could
> even register multiple handlers with different recipient (object) names.
> 
> I do not see the necessity to chain handlers for the same object. In
> my opinion this would also raise the complexity level and my intention
> was to keep it as simple as possible.

Well, do we want an object oriented interface, where operations are
performed on devices, streams, loopback instances, etc. or do we want a
function oriented interface, where there are just plain functions
grouped by API providers (where there is often, but not always, a 1:1
relationship between modules and API providers)? Object oriented
programming is the "mainstream" approach, and D-Bus has that design
too, but functional programming is popular too, so I don't think
there's any clear "right answer".

When access control is added, the access control policy will be
interested in the function/method name, and often it has to check the
operand object's attributes too. I imagine it would be easier for the
access policy if the messages contained the object name without needing
to parse it from the function parameters, but probably the difference
isn't huge if the operand objects are always the first function
parameters, so I don't have very strong opinion.

For example, let's say that there's a "set_latency" function that
operates on a loopback instance. The access control policy will get
either the following information:

object_name: "foo_loopback"
method_name: "set_latency"
parameters: "1234"

or

api: "loopback"
function_name: "set_latency"
parameters: "foo_loopback 1234"

The first case doesn't involve any parsing work to get the object name,
the second case does.

> > > > D-Bus has three message types: method calls, method replies and
> > > > signals. I think that's worth copying.
> > > 
> > > Method replies are just asynchronous replies to a previous
> > > method call. I do not see the need here since replies are sent
> > > synchronously. Or do you suggest to implement asynchronous
> > > replies?
> > 
> > What do you mean by "replies are sent synchronously"?
> 
> I mean synchronously from the perspective of the message handler.
> It receives some input and produces some output. It does not return
> without reply like a pure method call would, so currently there is only
> one type of message which is like a method call followed by a method
> reply. Therefore I see no need for other types of messages apart
> from signals.

I didn't mean that you'd need to add a new "method reply" type of
message. In fact, I said: "We already have a reply command, so that
probably doesn't need to be added." Your code already sends reply
commands from the server to the client, that's equivalent to the
"method reply" messages in D-Bus.

> > I don't see any
> > conceptual difference between how D-Bus works and how your protocol
> > works in this regard. From the application's point of view the messages
> > are asynchronous, that is, pa_context_send_message() doesn't wait for
> > the reply before returning. Maybe you mean that the reply is sent from
> > the message handler, but that's how D-Bus works too (replies don't have
> > to be sent from the message handler, but that's how it's often done).
> 
> The conceptual difference is that an explicit "method reply" type
> of message is not needed because each message gets a reply.
> 
> > 
> > > What would be the function of a signal? To notify all clients of some
> > > event? Or should all command handlers receive the signal if a client
> > > sends one? Signals are not intended for a specific recipient.
> > 
> > Signals would be used to notify clients of some event. There's no
> > reason why clients couldn't send signals too, but currently I don't
> > have any use cases for that, so it doesn't have to be supported yet.
> 
> Signal support should be the subject of another patch set. It would
> probably be easy to send, but there is a new notification interface
> that needs to be invented before it makes sense to send any signal.

Ok.

> > > > We already have a reply command, so that probably doesn't need
> > > > to be added.
> > > > 
> > > > Your proposal adds two different "method call" kind of messages, but I
> > > > don't really see why that is necessary. Every command will anyway have
> > > > at least a simple ack reply, so it's not like you're saving any
> > > > traffic. Commands that don't return anything could just respond with an
> > > > empty string.
> > > 
> > > You are right, the two message types are not strictly necessary.
> > > When I wrote the code, I was thinking about a "set parameter"
> > > and a "get parameter" command and the two message types
> > > reflect that. I still believe them to be useful to save some logic within
> > > the command handler to distinguish between the two types of
> > > commands, but if you think I should remove the no-reply variant,
> > > I can do it.
> > 
> > Yes, I think you should remove it.
> > 
> > By the way, it might be a good idea to improve the error reporting
> > mechanism. Currently the server can only send an error code, which
> > means that e.g. pactl often has quite uninformative error messages. So
> > far pactl has been able to at least validate the arguments to some
> > extent, but if the message parameters are completely opaque to pactl,
> > the error messages will be even worse.
> > 
> > I'd like to extend the current error command to include two strings: a
> > machine-readable string identifier and a human-readable error message.
> > The string identifier would have the same role as the current error
> > code, but it would be easier to add new string identifiers than new
> > error codes (adding new error codes requires extending the
> > pa_error_code_t enum). The human-readable error message would be a
> > bigger improvement: the server could actually send detailed information
> > about what went wrong.
> 
> Yes, I noticed that the error reporting is rather limited, but this would
> in my opinion also be a subject for a different patch set.

Ok.

-- 
Tanu

https://www.patreon.com/tanuk