Re: [pve-devel] [PATCH proxmox-perl-rs] initialize logging when shared library is loaded

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Thu, Feb 16, 2023 at 03:22:20PM +0100, Lukas Wagner wrote:
> Before, all calls to the log::* macros did not produce any output.
> This commit sets up logging by hooking into module loading/bootstraping
> process to call a single exported function `setup_logging`. The function
> initializes env_logger with its default settings. The benefit of this
> approach is that no changes of client-side Perl code is required.
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
> Not sure if this is the best/cleanest approach for this,
> seems to work just fine though.
> 
>  Proxmox/Lib/template.pm | 7 +++++++
>  common/src/logging.rs   | 8 ++++++++
>  common/src/mod.rs       | 1 +
>  pmg-rs/Cargo.toml       | 1 +
>  pve-rs/Cargo.toml       | 1 +
>  5 files changed, 18 insertions(+)
>  create mode 100644 common/src/logging.rs
> 
> diff --git a/Proxmox/Lib/template.pm b/Proxmox/Lib/template.pm
> index d7fbbf3..4942a64 100644
> --- a/Proxmox/Lib/template.pm
> +++ b/Proxmox/Lib/template.pm
> @@ -47,6 +47,13 @@ sub load : prototype($) {
>      $data->{$mod_name} = $lib;
>      $data->{-current} //= $lib;
>      $data->{-package} //= $pkg;
> +
> +    # Lookup up the 'setup_logger' function and call it.
> +    # This sets up 'env_logger' so that 'log::*' can be used from Rust code.
> +    my $sym  = DynaLoader::dl_find_symbol($lib, "setup_logging");
> +    die "failed to locate 'setup_logging'\n" if !defined $sym;
> +    my $setup_logging = DynaLoader::dl_install_xsub("setup_logging", $sym, "src/FIXME.rs");
> +    $setup_logging->();

A few notes:

There's an instance of this template by product, so one for PVE and one
for PMG. While loading both is unlikely (and probably a bad idea), it
should still be safe, so any one-shot initialization you do should
depend on that not having happened yet.

Above here there's code to set $::{'proxmox-rs-library'}, we should only
execute such one-shot initializers if this has not been set yet to make
sure loading PVE + PMG modules (unlikely but not impossible) doesn't
call the same code.

However, I don't think we should add custom-imported functions this way
at all.

I think instead of doing this manually it would make more sense to
actually use #[perlmod::package] to create a `Proxmox::Lib::PVE` and a
`Proxmox::Lib::PMG` package (in their corresponding lib.rs), give those
an `#[export] fn init() { ... }` and have that call out to the common
setup code, as we may want more initialization in the future without
having to keep modifying the template all the time.

To run that, we'd still need extend the template once. In the `BEGIN`
block at the bottom, after calling `__PACKAGE__->load()` we also need to
do:
    __PACKAGE__->bootstrap()
    init();

(Note the lack of `__PACKAGE__->` in front of the init() - the
`bootstrap()` call already pulled it in scope, and the `->` would just
append the package name as a string parameter.)

>  }
>  
>  sub bootstrap {
> diff --git a/common/src/logging.rs b/common/src/logging.rs
> new file mode 100644
> index 0000000..772b806
> --- /dev/null
> +++ b/common/src/logging.rs
> @@ -0,0 +1,8 @@
> +
> +#[no_mangle]
> +/// Should be called exactly once
> +pub extern "C" fn setup_logging() {

Now as a side note (since this won't be exported this way afterwards):
It's generally not recommended to export functions this way.
Better use `#[perlmod::export]` on it and load the function name with an
`xs_` prefix on it.
(While functions without parameters aren't really an issue there, the
macro also makes it easy to include errors and should in the future
probably also wrap the whole thing in a `catch_unwind()` and `croak()`
on panics back to perl...)

> +    if let Err(e) = env_logger::try_init() {
> +        eprintln!("could not set up env_logger: {e}")
> +    }
> +}