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.
