rnp icon indicating copy to clipboard operation
rnp copied to clipboard

Update set_rnp_log_switch() to allow logging fd.

Open ni4 opened this issue 5 years ago • 7 comments

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.

ni4 avatar Jun 02 '20 10:06 ni4

I can do this

rrrooommmaaa avatar Jun 02 '20 10:06 rrrooommmaaa

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

ni4 avatar Jun 02 '20 11:06 ni4

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.

dewyatt avatar Jun 03 '20 16:06 dewyatt

@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?

ni4 avatar Jun 03 '20 18:06 ni4

@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).

dewyatt avatar Aug 19 '20 02:08 dewyatt

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

bqv avatar Jan 25 '22 11:01 bqv

@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.

ni4 avatar Jan 25 '22 11:01 ni4