Re: [PATCH v3] genl: Add unicast handler

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1599 bytes --]

Hi Tim,

 >>
>> So to me this pid argument seems unnecessary.  Also, this change is side-
>> effecting l_genl_new(int fd) method which you are removing.  While nobody
>> is using it at the moment, removing it in this fashion is poor style (since
>> l_genl_set_close_on_unref is mostly meant for this constructor)
>
> FYI: This was your suggestion to my original patch .

Was it?  Don't recall.  I was probably under the assumption that pid 
setting was required.  I do reserve the right to change my mind, 
especially when looking at things more closely ;)

I'm picking on the procedural aspect.  E.g. if we remove something, then 
we should make sure it is done thoroughly (maybe even in a separate 
commit) and hopefully mention it in the commit description.

Once you start using git blame and hunting for why a piece of code looks 
the way it looks, having nice git history becomes paramount.

>>>
>>> +bool l_genl_set_unicast_handler(struct l_genl *genl,
>>> +						l_genl_msg_func_t handler,
>>> +						void *user_data,
>>> +						l_genl_destroy_func_t
>> destroy);
>>> +
>>> +bool l_genl_unset_unicast_handler(struct l_genl *genl);
>>
>> I rather we simply use l_genl_set_unicast_handler with NULL handler
>> argument to accomplish this.
>
> I was thinking about it, but then the API does not look self-explanatory.
>

This is inconsistent with the rest of ell API though.  For example, see 
l_io_set_read_handler, l_io_set_write_handler, 
l_io_set_disconnect_handler, etc.  Any clarifying details can also be 
added to the docs.

Regards,
-Denis