Re: [pbs-devel] [PATCH-SERIES] replace print by log macro in libraries

[Hannes Laimer <h.laimer@proxmox.com>]

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 [1] 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...)

[1] 
https://git.proxmox.com/?p=proxmox-backup.git;a=blob;f=pbs-client/src/pxar/fuse.rs;h=0b90ff2ce36c87b2a7fbd208e581c33700a7e9e1;hb=refs/heads/master#l299