CESNET / libnetconf2

C NETCONF library
BSD 3-Clause "New" or "Revised" License
202 stars 146 forks source link

`nc_set_print_clb_session` is not really a per-session callback #476

Closed jktjkt closed 5 months ago

jktjkt commented 5 months ago

When porting libnetconf2-cpp to the new API, we found out that the callback remains active even after the session has been destroyed. When a new, fresh nc_session is created, libyang would still log to the old CB with the session parameter set to NULL. I do not have a C reproducer for you, but that's essentially what our code is doing.

I suggest either:

jktjkt commented 5 months ago

After reading through libnetconf2's code, it seems that all libyang messages are logged with a NULL nc_session* once a NETCONF-level logger is set (it was pure chance that the test did not hit that code path before). I suggest documenting that, or, preferably, not setting a logger of another library at all.

michalvasko commented 5 months ago

changing libyang so that logging CB is set per context (the best option IMHO)

Not every message has a context available and I think we should stick to the common logging design when the set callback are global for all the messages of a library.

changing libnetconf2 so that its log CB doesn't change the libyang's log CB (the second best option for me)

I have no strong opinion and nothing against this change really, netopeer2 sets a callback for all the libraries.

documenting nc_set_print_clb_session that...

No problem, when the callback was added it seemed obvious that just the session as a parameter was added with no additional changes but that is no longer the case so it can be described in more detail.