public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Wolfgang Bumiller <w.bumiller@proxmox.com>
To: Lukas Wagner <l.wagner@proxmox.com>
Cc: pve-devel@lists.proxmox.com
Subject: Re: [pve-devel] [PATCH proxmox-perl-rs] initialize logging when shared library is loaded
Date: Fri, 17 Feb 2023 14:21:58 +0100	[thread overview]
Message-ID: <20230217132158.xzoslmizi2el3jdg@fwblub> (raw)
In-Reply-To: <20230216142220.886551-1-l.wagner@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}")
> +    }
> +}




      reply	other threads:[~2023-02-17 13:22 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-16 14:22 Lukas Wagner
2023-02-17 13:21 ` Wolfgang Bumiller [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230217132158.xzoslmizi2el3jdg@fwblub \
    --to=w.bumiller@proxmox.com \
    --cc=l.wagner@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal