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

[Georg Chini]

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?

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

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

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

>
>>> 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?
> Yes, it would be another notification system (in case it was not clear,
> I'm talking about signals here). The existing subscription API can't be
> extended by modules, and it also has the problem that it only supports
> three kinds of events: new objects, removed objects and changes in
> object state. The "bluetooth headset button pressed" event doesn't fit
> that model, for example.

The "button press" event is a very good example for the use of a signal.

>
>>> 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.
> My opinion is that additions to the core API should not deviate from
> the existing core API conventions, so applications would use this new
> message interface to only deal with module specific features. However,
> if it seems beneficial, the implementation behind the libpulse API
> could use this protocol also for core features.
>
Mh, if the message interface should only deal with module specific
features like I originally intended, I wonder why you object to a
module-centric approach. If a module implements multiple handlers
for different objects they could be referenced as module.object_name.

"Core" could be treated just like another module and module plug-in's
like objects within a module.

But never mind ...