Introduce a new krun_init_log() API #326
Conversation
Introduce a new krun_init_log public API function to allow for much more detailed configuration of logging by the application. The main improvment is is the ability to specify a file descriptor to write the log to. Signed-off-by: Matej Hrica <[email protected]>
chroot_vm is meant as an example and program to showcase libkrun APIs, we should at least show error messages by default even without RUST_LOG env variable. Signed-off-by: Matej Hrica <[email protected]>
Use the newer krun_init_log to support redirecting the log to a pipe. Signed-off-by: Matej Hrica <[email protected]>
The correct way to use the env_logger crate is to only depend on it the toplevel aplication crate. In other crates we should just use the `log` crate facade (which we already do). Drop the env_logger dependency from all of our internal crates, and just keep it in the the `libkrun` crate. Signed-off-by: Matej Hrica <[email protected]>
Use newest version of env_logger. This is needed to fix a problem where output to a pipe ignores RUST_LOG_STYLE=always option and colors don't work. Signed-off-by: Matej Hrica <[email protected]>
| * Returns: | ||
| * Zero on success or a negative error number on failure. | ||
| */ | ||
| int32_t krun_init_log(int target_fd, const char *level, uint32_t style, uint32_t options); |
There was a problem hiding this comment.
I know this is still a draft, but is this new API meant to be used instead of krun_set_log_level? i.e. are we deprecating the old API
There was a problem hiding this comment.
I'm not sure, should we? Both can work at the same time, but this one is more flexible. If we get to having a v2 API, we should probably only keep this one.
One confusing thing about the existing krun_set_log_level, is that if you don't call it RUST_LOG env variable will not work - you have to call something like krun_set_log_level(0) to initialize the logging.
There was a problem hiding this comment.
Like you said, I think deprecating krun_set_log_level would make sense for v2.
If for some reason we don't deprecate it, I would like to see krun_init_log use an integer for the log level instead of a string to be uniform.
There was a problem hiding this comment.
I agree, please use an integer for "level", and allow arbitrarily large values to be treated as "trace".
There was a problem hiding this comment.
Sure, I can make it an integer. But just to clarify the reason I made level a string - it is so that you can pass in the arbitrary RUST_LOG options. e.g. level = "devices::virtio::gpu=trace". Though I'm not sure if this is useful, since you could still use the env variable to set that...
Introduce a new krun_init_log public API function to allow for much detailed configuration of logging by the application. The main improvment is the ability to specify a file descriptor to write the log to.
This can be tested using the added
chroot_vmflags:--log=/path/to/a/pipeor--color-log=/path/to/a/pipe(which will also display a formatting).