From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 57DD79319B for ; Fri, 17 Feb 2023 14:22:04 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 40E786C08 for ; Fri, 17 Feb 2023 14:22:04 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 17 Feb 2023 14:22:00 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 57BE847700 for ; Fri, 17 Feb 2023 14:22:00 +0100 (CET) Date: Fri, 17 Feb 2023 14:21:58 +0100 From: Wolfgang Bumiller To: Lukas Wagner Cc: pve-devel@lists.proxmox.com Message-ID: <20230217132158.xzoslmizi2el3jdg@fwblub> References: <20230216142220.886551-1-l.wagner@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230216142220.886551-1-l.wagner@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.187 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH proxmox-perl-rs] initialize logging when shared library is loaded X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 17 Feb 2023 13:22:04 -0000 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 > --- > 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}") > + } > +}