On Sun, 2017-10-01 at 20:31 +0200, Georg Chini wrote: > On 01.10.2017 18:16, Tanu Kaskinen wrote: > > On Sat, 2017-08-19 at 17:48 +0200, Georg Chini wrote: > > > +/* List handlers */ > > > +char *pa_core_message_handler_list(pa_core *c); > > > > Putting this function to core-messages.h doesn't seem right to me. The > > function will never be used outside core.c, so the it should be a > > private function in core.c. > > > > If I put it in core.c, I would need to expose the pa_core_message_handler > structure, which you wanted to keep private. I think it actually makes sense to make the struct public. It could be private if it didn't have the description field, but since the handler objects contain information that clients can query, they're similar to all other core objects. I think it even makes sense to move the struct to a separate message-handler.h header (but if you don't like that idea, I'm fine with keeping it in core-messaging.h or moving it to core.h). > Also I am not sure if the > function > will never be called from outside core.c. It might be useful to know, > which message handlers are present. pa_core_message_handler_list() returns a string whose format doesn't have a specification, so it's not possible to reliably get the message handler information that way. And parsing the string would be more cumbersome anyway than having the information directly accessible via the pa_core_message_handler struct if that's made public. Something that didn't occur to me when I reviewed this patch yesterday is that the string that the list-handlers returns is tailor-made for pactl/pacmd, which is not how the message handlers are expected to behave. What if an application wants to not only print a message (of which layout is already decided by the server), but to get a list of handlers and do something else with that information? The list-handlers command should return machine-readable information: either only a list of the object paths (with the possibility to query the description of a handler with another message), or a dictionary whose keys are object paths and values are dictionaries representing the handler objects. The per-object dictionary would contain property names as keys (currently the description is the only property) and property values as variant values (by variant I mean the D-Bus definition of the word). Returning a list containing plain (path, description) pairs wouldn't be a good idea, because that would cause trouble when adding new properties to the handler objects. Also, it's questionable why the list-handlers command is implemented with the messaging API in the first place. It's core functionality, so why is it not implemented using the normal introspection API? And one more thing: if the handler descriptions are visible to clients and the descriptions are mutable, clients should be notified about description changes. -- Tanu https://www.patreon.com/tanuk
