[Review] feat(pubsub): change std_log.h into log.h, implement configurable loger for pubsub_ethernet module

[saukijan]

Description

This PR removes the use of the standard log_stdout.h from ua_pubsub_ethernet.c and implements the use of a more generalized log.h in its place.

Simmilarly to PR#3567 this is achieved by using a static UA_Logger variable, which is initially set to NULL and set during configuration when setting the transport layer by UA_PubSubTransportLayerEthernet(&config->logger)

A NULL logger is acceptable since log.h checks against it, and if it is found, no logging is done, which in principal, allows for full deactivation of the logging plugin during the configuration process.

[jpfr]

Hey,

Thanks for the effort. I have a few requests from a maintainer point.

• don’t rename to log.h This is one of several possible loggers. Log_stdout is not bad for a logger that writes to stdout.
• don’t use static variables. Several servers can run in parallel in the same process.
  We don’t use non-constant static variables ever.
• split essential changes and cosmetic changes in separate commits.
  This is impossible to review otherwise.

That said, using the configured logging is very welcome.

[saukijan]

Hi @jpfr ,

Thanks for your feed back! I addressed the issues you mentioned:

• split essential changes and cosmetic changes in separate commits.
  This is impossible to review otherwise.

I removed the clang-formater changes and left in only the PR relevant changes themselves.

don’t use static variables. Several servers can run in parallel in the same process.
We don’t use non-constant static variables ever.

I think my initial comment was not really clear. This change uses a static const UA_Logger *, which allows for the pointer itself to be changed, but not it's value and the only way to change the pointer is if entire PubSub Transport Layer is rebuilt.

• don’t rename to log.h This is one of several possible loggers. Log_stdout is not bad for a logger that writes to stdout.

I am not sure what you mean by this, log.h is the header that contains the definition for UA_Logger struct, which declares the callback footprints that all of the loggers will use, where as log_stdout.h implements said callbacks.

Correct me if I am wrong, but to me this is a typical Interface and the point of an interface is to separate concrete implementation of a functionality from the user of that functionality.

Thus if log_stdout.h header stays, there is no way to implement a configurable logger, since UA_Log_Stdout will be used regardless of what is set within UA_ServerConfig structure.

Furthermore, this change still allows for the use of UA_Log_Stdout as a logger. Instead of using a direct declaration within ua_pubsub_ethernet.c, this uses the logger that was given during PubSub Transport Layer creation (the call of UA_PubSubTransportLayerEthernet() function) and by default this is set to UA_Log_Stdout by functions like UA_ServerConfig_setDefault(UA_ServerConfig *config)

[saukijan]

@jpfr, this PR is ready for review

[saukijan]

Ill re-add the log_stdout.h and try to do something similar to PR #3567 and contexts

[saukijan]

Hey @jpfr, I moved const UA_Logger * logger to UA_PubSubConnectionConfig struct, however I am unsure who sets it and how TransportLayerEthernet_addChannel(UA_PubSubConnectionConfig *connectionConfig) is called with UA_PubSubConnectionConfig parameter.

[jpfr]

The core of the change looks good.
There are some artifacts in the PR that are not needed.

• Changes to submodules
• UA_Boolean configurationFrozen;

Can you clean up the PR and rebase? Then we can merge.

[saukijan]

sure thing, I'll commit the fixes next week

[saukijan]

sorry for the long delay, applied the changes, ready for review