[pbs-devel] [PATCH-SERIES] replace print by log macro in libraries
h.laimer at proxmox.com
Fri Mar 18 07:02:11 CET 2022
Am 17.03.22 um 09:40 schrieb Wolfgang Bumiller:
> I like the direction of this.
> On Fri, Mar 11, 2022 at 03:07:45PM +0000, Hannes Laimer wrote:
>> This series mostly replaces print with the log macro in libs, it also replaces print
>> in binaries where it is used to log stuff and not output the result of a command.
>> In the process of replacing prints by log macros a few parameters controlling verbosity
>> became obsolete and were removed, other 'verbose' parameters influenced the control
>> flow and where therefore kept.
> Where does verbosity affect control flow? That sounds strange.
> And I don't think we should just *drop* `--verbose` parameters. I do
> think it would be good to *have* them, either by promoting
> previously-verbose output to `log::debug` and making the parameter
> affect the filter, or by using a task-local variable we don't need to
> hand down through all the function calls, though the latter might be a
> bit more involved (given that eg. tokio's LocalKey is not inherited
> across `spawn()`...)
Logging output that was behind a `if verbose..` was replaced with a
log::debug, I should have mentioned that in the summary. If the
`verbose` parameter was used to define the commands result(e.g.
proxmox-backup-manager version) it was kept.
In  we only return if self.verbose is true, looking at it again maybe
renaming verbose to foreground(or smth similar) might make sense here...
but not sure.
>> The whole changes were split up into 7 seperate patches[3-9], this was done
>> to aviod one huge patch file and improve readability. Those (maybe also 2)
>> should be squashed when applied since they are not necesarilly buildable.
>> The reason for that is that in a few places 'verbose' parameters were remove.
>> A verion bump is also needed since patches 2 (and indirectly 3-10) depend on
>> the function added to proxmox-router in patch 1.
> As for the helper... I'm a bit unsure here.
> We currently always pass "info", and we use "PBS_LOG" as env var
> everywhere except the pxar binary.
> While on the one hand flexibility would be nice... I think we could
> also just drop the parameters (or make them `Option`s)?
I wanted to keep it as general as possible, since it is in a not PBS
specific crate, but Option makes sense for the verbosity level, env var
name 'PBS_LOG' outside of PBS seems out of place.
(init_cli_logger('LOGLVL', None) might look a little weird though...)
More information about the pbs-devel