raito/lanzaboote

Fork 0

tool: init configurable logging #121

Merged

nikstur merged 1 commit from tool-configurable-logging into master

2023-03-05 22:27:12 +00:00

nikstur commented

2023-02-26 23:31:40 +00:00

(Migrated from github.com)

Replace println() calls with logging at different levels using the log crate.

Closes #55 #73

Replace `println()` calls with logging at different levels using the `log` crate. Closes #55 #73

blitz (Migrated from github.com) reviewed

2023-02-27 11:06:47 +00:00

rust/tool/src/logging.rs

blitz (Migrated from github.com) commented

2023-02-27 11:06:47 +00:00

Typically logging goes to stderr to avoid buffering log messages. But at that point, you just re-implement stderrlog. Why not use that? It also gives you the typical Unix behavior of -v, -vv, -vvv.

Typically logging goes to stderr to avoid buffering log messages. But at that point, you just re-implement [stderrlog](https://docs.rs/stderrlog/0.5.4/stderrlog/). Why not use that? It also gives you the typical Unix behavior of `-v`, `-vv`, `-vvv`.

blitz (Migrated from github.com) reviewed

2023-02-27 11:08:30 +00:00

rust/tool/src/signature.rs

blitz (Migrated from github.com) commented

2023-02-27 11:08:30 +00:00

This sounds like error! or at least warn!? I mean if this is not an error what is? :) In case of errors, it also makes sense to dump all information required for debugging that you have at hand immediately.

This sounds like `error!` or at least `warn!`? I mean if this is not an error what is? :) In case of errors, it also makes sense to dump all information required for debugging that you have at hand immediately.

blitz (Migrated from github.com) reviewed

2023-02-27 11:09:18 +00:00

rust/tool/src/signature.rs

blitz (Migrated from github.com) commented

2023-02-27 11:09:18 +00:00

But maybe the log message should be at the call site of verify, otherwise legitimate uses of verify might spam the log?

But maybe the log message should be at the call site of `verify`, otherwise legitimate uses of verify might spam the log?

blitz (Migrated from github.com) requested changes

2023-02-27 11:16:01 +00:00

blitz (Migrated from github.com) left a comment

I would strongly suggest to just use stderrlog. It brings minimal dependencies and has functionality that is actually useful.

(Optional)I would also encourage you to write a small piece of documentation somewhere about the logging policy. Maybe we start a developer guideline?

My personal view is (up for debate):

error -> "Something definitely went wrong"
warn -> "Something went wrong, but the end result might still be ok."
info -> "This is truly interesting information to the user."
verbose -> "Logs actions that are taken at a high-level. Should still be decipherable by a user"
debug -> "Logs additional details about those actions that are only useful for debugging for developers. This doesn't need to be decipherable by users. Should not spam the log."
trace -> "Really annoying/invasive logging, that might occasionally come in handy"

I would strongly suggest to just use `stderrlog`. It brings minimal dependencies and has functionality that is actually useful. (Optional)I would also encourage you to write a small piece of documentation somewhere about the logging policy. Maybe we start a developer guideline? My personal view is (up for debate): - error -> "Something definitely went wrong" - warn -> "Something went wrong, but the end result _might_ still be ok." - info -> "This is truly interesting information to the user." - verbose -> "Logs actions that are taken at a high-level. Should still be decipherable by a user" - debug -> "Logs additional details about those actions that are only useful for debugging for developers. This doesn't need to be decipherable by users. Should not spam the log." - trace -> "Really annoying/invasive logging, that might occasionally come in handy"

nikstur (Migrated from github.com) reviewed

2023-02-28 19:23:49 +00:00

rust/tool/src/signature.rs

nikstur (Migrated from github.com) commented

2023-02-28 19:23:48 +00:00

Right now this function is called only to check if the systemd-boot binary is signed or not. There, it should not be error because we just update the binary if it not signed (e.g. because you just installed Lanzaboote). So you're right it should be at the call site. Maybe a warning makes more sense than an info. But there is really nothing for the user to do, that's why it maybe should be an info?

Right now this function is called only to check if the systemd-boot binary is signed or not. There, it should not be `error` because we just update the binary if it not signed (e.g. because you just installed Lanzaboote). So you're right it should be at the call site. Maybe a warning makes more sense than an info. But there is really nothing for the user to do, that's why it maybe should be an info?

nikstur (Migrated from github.com) reviewed

2023-02-28 19:28:41 +00:00

rust/tool/src/logging.rs

nikstur (Migrated from github.com) commented

2023-02-28 19:28:41 +00:00

Never heard of the stderrlog crate before but it looks good. I'm not too fond of the multiple -vvv flags, since I do not believe that a normal CLI user would be interested in anything besides normal output and debug output. However, since Lanzaboote is not really a normal CLI, as it is only called by a module, we might as well implement all log levels.

nikstur commented

2023-02-28 21:11:53 +00:00

(Migrated from github.com)

Thanks for the review @blitz! I think we can/should address the logging policy separately. I opened an issue for a contributors guide #124. We should follow the Levels from the log crate IMO

Thanks for the review @blitz! I think we can/should address the logging policy separately. I opened an issue for a contributors guide #124. We should follow the [`Level`](https://docs.rs/log/latest/log/enum.Level.html)s from the log crate IMO

👍 1

nikstur (Migrated from github.com) reviewed

2023-02-28 21:12:23 +00:00

rust/tool/src/install.rs

nikstur (Migrated from github.com) commented

2023-02-28 21:12:22 +00:00

I don't love this. Do you have any idea how to make this more concise?