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

[Georg Chini]

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?

>
> 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 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.

>
> 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.

>
> Your proposal doesn't have a way to send notification messages from the
> server to the client. I think this is rather important to have (but
> this can also be added in a later patch).

Wouldn't that mean to implement another notification system apart
from the subscription API? Or do you think the subscription could
be extended?

>
> I wrote and deleted some stuff about things that D-Bus has: interfaces,
> signatures and introspection. We may want those at some point, and I
> thought that the basic message protocol would need some changes to be
> compatible with them, but I don't think that's the case after all.
>
> In conclusion, I have much less complaints about the high-level idea
> than I first thought I would have (I didn't read the code in detail).
> Just avoid making the protocol module-centric and remove the redundant
> command-without-reply message type. Signal support would be nice too.
>
> This new protocol has the capability to be used not only as a
> replacement for the extension system, but as a replacement for
> everything else in the native protocol too. I'd like to have some
> discussion about what the new protocol's scope should be going forward
> (I added Arun to Cc for this reason). Should new core features be added
> using this mechanism, or should we keep adding new commands to the
> native protocol as we've done so far?
>
Yes, I would also like some discussion to make sure that I understand
the scope correctly.