On 02.10.2017 11:31, Tanu Kaskinen wrote: > 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). You want a header file just for this struct? I would keep it in core-messages.h but if you prefer, I don't mind using a separate file. > >> 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. If the struct is made public, the function is simply obsolete. The core contains a pointer to the hashmap. Formatting the string can then be done directly in the message handler. > > 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? I don't understand. Message handlers return a single string, so parsing the result is the task of the sender. The format returned by the handler is rather simple, apart from the header line it only consists of lines of the form handler-path, " ", description This should be easy to parse. > 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. Again I don't understand, see above. list-handlers is a message sent to the core, not a command. As such, it returns a string. > > 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? Two reasons: a) It's much simpler than using the introspection API b) We should at least have one example in the code which shows how a message handler i implemented. > And one more thing: if the handler descriptions are visible to clients > and the descriptions are mutable, clients should be notified about > description changes. > How? Sending a signal?
