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

[Review] feat(plugin): implement configurable logger for pki_default unit #3567

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

saukijan
Copy link

Description

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

This is achieved by using a static UA_Logger variable, which is initially set to NULL and set during configuration by one of the following functions:

  • UA_CertificateVerification_AcceptAll()
  • UA_CertificateVerification_Trustlist()
  • UA_CertificateVerification_CertFolders()

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.

@saukijan saukijan changed the title [Review] feat(plugin): implement configurable logger for pki_default unit [WIP] feat(plugin): implement configurable logger for pki_default unit Apr 24, 2020
…t for openssl implementations in ua_pki_openssl module

Signed-off-by: Dovydas Girdvainis <[email protected]>
@saukijan
Copy link
Author

This PR needs to change both the default and openssl PKI modules to pass CI tests, thus resolving the plugins/securityPolicies/openssl/ua_pki_openssl.c PR mentioned in Issue #3544 as well

@saukijan saukijan changed the title [WIP] feat(plugin): implement configurable logger for pki_default unit [Review] feat(plugin): implement configurable logger for pki_default unit Apr 28, 2020
@saukijan
Copy link
Author

This PR is ready for review

@jpfr
Copy link
Member

jpfr commented May 20, 2020

Please move the logger from a global pointer into the CertContext struct.
We avoid use the use of non-const global variables in the library.

Good to merge otherwise!

@saukijan saukijan changed the title [Review] feat(plugin): implement configurable logger for pki_default unit [WIP] feat(plugin): implement configurable logger for pki_default unit May 22, 2020
…t, add missining CertContext instantiantion and clean up, fix CertContext usage

Signed-off-by: Dovydas Girdvainis <[email protected]>
…logger ptr to CertContext for ua_pki_openssl

Reanme ContextCert to DefaultContetxtCert for ua_pki_default.c to avoid definition conflicts with CertContext within ua_pki_openssl

Signed-off-by: Dovydas Girdvainis <[email protected]>
@saukijan saukijan changed the title [WIP] feat(plugin): implement configurable logger for pki_default unit [Review] feat(plugin): implement configurable logger for pki_default unit May 22, 2020
@saukijan
Copy link
Author

saukijan commented May 22, 2020

Hey @jpfr, thanks for your input.

Please move the logger from a global pointer into the CertContext struct.

I did not notice the ContextCert before, it seems a much better fit than global ptr! I used the openssl module definition as an example and created DefalutContextCert structure, to differentiate it from the one defined in ua_pki_openssl.c

I also pulled in the latest commits from master, but that seems to have broken unit test runner for me. I keep getting errors from Python telling me that Valgrind logfile do not exist. Here is the error log:
ctest.log

Also azure pipeline keeps failing to download some dependencies for windows setup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants