Logging API changes break API and ABI of libosdp

[sidcha]

Commit c9f3f98 breaks the API and ABI of libosdp because the signature of osdp_logger_init changes. I would expect that to be a bump in the major version of the library.

personal preference: don't try to format the entire log message before calling the log function. In C++ people may be using a logging framework like log4cxx or spdlog which already have log levels, line numbers, date/time formatting. So from libosdp we get a message like:

osdp: [2023-10-09T13:51:23] [INFO ] src/osdp_pd.c:1126: Setup complete - libosdp-2.3.0 osdp_pd_status (dee622a)

which could then be formatted by a logging framework to something like:

[2023-10-09T13:51:23] [INFO] libosdp - osdp: [2023-10-09T13:51:23] [INFO ] src/osdp_pd.c:1126: Setup complete - libosdp-2.3.0 osdp_pd_status (dee622a)

I have a simple logger that can be used as a starting point/inspiration: https://github.com/rm5248/simplelogger

Reported-by: @rm5248

[sidcha]

Commit c9f3f98 breaks the API and ABI of libosdp because the signature of osdp_logger_init changes. I would expect that to be a bump in the major version of the library.

This is now fixed. I will add a doc to formalise stable API change process.

don't try to format the entire log message before calling the log function. In C++ people may be using a logging framework like log4cxx or spdlog which already have log levels, line numbers, date/time formatting.

The expectation is to let libosdp do all log formatting and level management; the app pass a raw printf() like method that writes directly to whatever medium you'd like to output the logs to (files, uart, etc.,). The rational for taking this approach is simple; there is no one accepted logging framework for C/C++ (I've seen way too many ad-hoc implementations). So any form if generalization we introduce for one library will fall short for another (some embedded systems have none).

We could probably have a lean log reporting method that just gets called with the log level and log message without any processing and let the app choose which logging framework to initialise.

[rm5248]

We could probably have a lean log reporting method that just gets called with the log level and log message without any processing and let the app choose which logging framework to initialise.

That makes the most sense to me and gives the most flexibility.

FWIW what I've done in the past is to give the user the ability to set the log function that will be called, but also provide a sample implementation of that log function that will just print to stderr. This lets you quickly output some data to stderr, but the ability to use a comprehensive logging framework(if you are using one).

[sidcha]

FWIW what I've done in the past is to give the user the ability to set the log function that will be called, but also provide a sample implementation of that log function that will just print to stderr. This lets you quickly output some data to stderr, but the ability to use a comprehensive logging framework(if you are using one).

Thank you for the pointers. We have many examples of how to setup the logger within this repo. Do let me know if they are not easy to find/follow. I've also added a new API to set a log callback function as discussed here; let me know if you face any issues integrating it.

[rm5248]

Note: commit c932fbebdc481ac7cb1a98f5c4410df83a75ec1a breaks backwards ABI compatibility, because you have removed the function and turned it into a macro. The macro keeps the API compatible, but because the symbol no longer exists anything compiled against the old version will break at runtime(because it can't find the symbol).

ABI rules can be found here(this is for C++, but the same rules still apply for C): https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B

You cannot...

• For existing functions of any type: Unexport it. Remove it.

[sidcha]

Ahh, my bad. If you want ABI compatibility, does it make sense to stick to one of the old releases? In my development workflow, master has unstable changes (much like this logging infra change).

I'm preparing for a major release from master (may change a lot of stuff in osdp.h for rust support) and it's too restrictive if users are consuming the tip of master in production :)

[rm5248]

I am not using master, I just noticed the change.

If you're making a major release(e.g. a 3.x version) then breaking the ABI/API is fine; I was expecting the next version to be 2.4.0, something where I would not expect the API/ABI to break. This is what semver specifies.