rnpgp / rnp

RNP: high performance C++ OpenPGP library used by Mozilla Thunderbird
https://www.rnpgp.org
Other
194 stars 55 forks source link

Update set_rnp_log_switch() to allow logging fd. #1158

Open ni4 opened 4 years ago

ni4 commented 4 years ago

Description

Currently we are able to disable/enable logging. But it would be useful to be able to specify fd for logging output. That would allow to easily redirect log to file/pipe, and utilize this in tests and applications which use library.

rrrooommmaaa commented 4 years ago

I can do this

ni4 commented 4 years ago

@rrrooommmaaa Thanks. But it is not of high priority right now.

dewyatt commented 4 years ago

This is already available via rnp_ffi_set_log_fd for functions that use FFI_LOG, right? That was the original idea there, we just didn't update everything to use FFI_LOG yet.

ni4 commented 4 years ago

@dewyatt FFI_LOG() requires rnp_ffi_t instance, while RNP_LOG works on lower layers, where it will be unavailable. However, rnp_ffi_set_log_fd() could also set lower-layer global variable, what do you think about such approach?

dewyatt commented 4 years ago

@dewyatt FFI_LOG() requires rnp_ffi_t instance, while RNP_LOG works on lower layers, where it will be unavailable. However, rnp_ffi_set_log_fd() could also set lower-layer global variable, what do you think about such approach?

I lost track of this for a bit, but my thoughts are that it's probably not the best approach. Some appropriate logging context needs to be passed everywhere that logging is going to be done, even if seemingly burdensome. It's not OK to use a global or thread-local, it's only going to hinder debugging later on when trying to make sense of a log that has interleaved messages from multiple unrelated contexts within a given application (and we probably end up with a global mutex to synchronize that).

bqv commented 2 years ago

Much better would be just a function pointer to call for logging

ni4 commented 2 years ago

@bqv Thanks for the note, definitely makes sense. To not break current workflow probably we should have both, adding some logging context as @dewyatt suggested.