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. 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'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. D-Bus has three message types: method calls, method replies and signals. I think that's worth copying. 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. 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). 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? -- Tanu https://www.patreon.com/tanuk
