Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make logging a proper plugin with state and cleanup method #2263

Merged
merged 6 commits into from
Dec 4, 2018

Conversation

jpfr
Copy link
Member

@jpfr jpfr commented Dec 2, 2018

No description provided.

"Could not create client socket: %s", strerror(UA_ERRNO));
ClientNetworkLayerTCP_close(connection);
return UA_STATUSCODE_BADDISCONNECT;
UA_LOG_WARNING(UA_Log_Stdout, UA_LOGCATEGORY_NETWORK,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should use logger

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely.
I would go so far not to include #include "ua_log_stdout.h" here at all.

ClientNetworkLayerTCP_close(connection);
return UA_STATUSCODE_BADDISCONNECT;
if(UA_socket_set_nonblocking(clientsockfd) != UA_STATUSCODE_GOOD) {
UA_LOG_WARNING(UA_Log_Stdout, UA_LOGCATEGORY_NETWORK,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should use logger

@@ -634,7 +635,7 @@ UA_StatusCode UA_ClientConnectionTCP_poll(UA_Client *client, void *data) {
if ((error == -1) && (UA_ERRNO != UA_ERR_CONNECTION_PROGRESS)) {
ClientNetworkLayerTCP_close(connection);
UA_LOG_WARNING(UA_Log_Stdout, UA_LOGCATEGORY_NETWORK,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here and in the rest of the file

@mlgiraud
Copy link
Contributor

mlgiraud commented Dec 3, 2018

Couldn't we make the logger a singleton so that we can access it wherever we want to?
That way we wouldn't have to have all those pesky logger variables in all kinds of places.
IMHO that would clean up the code a bit.

Might make the logging calls easier as well. Example:

static UA_INLINE UA_FORMAT(3,4) void
UA_LOG_WARNING(UA_LogCategory category, const char *msg, ...) {
#if UA_LOGLEVEL <= 400
    UA_Logger logger* = UA_GetLogger();
    va_list args; va_start(args, msg);
    logger->log(UA_LOGLEVEL_WARNING, category, msg, args);
    va_end(args);
#endif
}
UA_LOG_WARNING(category, "my log message");

@jpfr
Copy link
Member Author

jpfr commented Dec 3, 2018

Couldn't we make the logger a singleton so that we can access it wherever we want to?

Singletons in C are global variables.
http://wiki.c2.com/?GlobalVariablesAreBad

Especially, global variables that are not a constant are bad...
You might have several instances of client / server in one process.
At which point can you shut down the (stateful) logger?

@jpfr jpfr force-pushed the stateful_logging branch 2 times, most recently from d381abc to 4af545d Compare December 3, 2018 13:14
@coveralls
Copy link

coveralls commented Dec 3, 2018

Coverage Status

Coverage decreased (-0.009%) to 85.059% when pulling 4229a2d on stateful_logging into 885bad0 on master.

@jpfr
Copy link
Member Author

jpfr commented Dec 4, 2018

@Pro After trying several times, I am unable to find the source of the problem on OSX.
It is related to the posix architecture definitions.

Do you have a hunch of how to fix this?

@Pro
Copy link
Member

Pro commented Dec 4, 2018

@jpfr this must be related to the removal of ua_log_stdout.h
I reverted that change in one commit (ad4f3e6) and now OS X is green, since the issue is not related to your changes. The macro redefinition is already happening on master, check e.g. https://travis-ci.org/open62541/open62541/jobs/462374570
Looks like it is not failing even though there are warnings and errors in the build.

So let's keep my commit in this branch and I will add a new PR which tries to solve the issue with the queue.h.

The PR is here:
#2268

@Pro Pro merged commit 3e43f61 into master Dec 4, 2018
@Pro Pro deleted the stateful_logging branch December 4, 2018 17:39
@lock lock bot locked as resolved and limited conversation to collaborators Dec 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants