[pve-devel] [PATCH proxmox 1/2] notify: smtp: add missing 'tracing' namespace prefix

[pve-devel] [PATCH proxmox 1/2] notify: smtp: add missing 'tracing' namespace prefix

[Lukas Wagner]

This section of code is only compiled when the 'mail-forwarder' feature
is enabled, which might have been the reason why this was missed when the
other places where log messages are produced were migrated to 'tracing'.

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 proxmox-notify/src/endpoints/smtp.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/proxmox-notify/src/endpoints/smtp.rs b/proxmox-notify/src/endpoints/smtp.rs
index 6bb2d2d0..86911f9b 100644
--- a/proxmox-notify/src/endpoints/smtp.rs
+++ b/proxmox-notify/src/endpoints/smtp.rs
@@ -336,7 +336,7 @@ impl Endpoint for SmtpEndpoint {
                                 let header = HeaderValue::new(name, value);
                                 message.headers_mut().insert_raw(header);
                             }
-                            Err(e) => error!("could not set header: {e}"),
+                            Err(e) => tracing::error!("could not set header: {e}"),
                         }
                     }
                 }
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH proxmox-mail-forward 2/2] switch to proxmox-log

[Lukas Wagner]

The proxmox-notify crate now uses tracing for logging,
hence we have to switch to proxmox-log (which configures the appropriate
layers/subscribers for tracing).

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 Cargo.toml  |  5 ++---
 src/main.rs | 16 +++++++---------
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 0f4e3b0..9dc081b 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -15,9 +15,8 @@ exclude = [ "debian" ]
 
 [dependencies]
 anyhow = "1.0"
-log = "0.4.17"
 nix = "0.26"
-syslog = "6.0"
 
-proxmox-sys = "0.6"
+proxmox-log = "0.2"
 proxmox-notify = {version = "0.5", features = ["mail-forwarder", "pve-context", "pbs-context"] }
+proxmox-sys = "0.6"
diff --git a/src/main.rs b/src/main.rs
index 4662ffa..c0ea561 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -23,6 +23,8 @@ use std::path::Path;
 
 use anyhow::Error;
 
+use proxmox_log::error;
+use proxmox_log::LevelFilter;
 use proxmox_notify::context::pbs::PBS_CONTEXT;
 use proxmox_notify::context::pve::PVE_CONTEXT;
 use proxmox_notify::Config;
@@ -42,7 +44,7 @@ fn attempt_file_read<P: AsRef<Path>>(path: P) -> Option<String> {
     match fs::file_read_optional_string(path.as_ref()) {
         Ok(contents) => contents,
         Err(err) => {
-            log::error!("unable to read {path:?}: {err}", path = path.as_ref());
+            error!("unable to read {path:?}: {err}", path = path.as_ref());
             None
         }
     }
@@ -112,11 +114,7 @@ fn forward_for_pbs(mail: &[u8], has_pve: bool) -> Result<(), Error> {
 }
 
 fn main() {
-    if let Err(err) = syslog::init(
-        syslog::Facility::LOG_DAEMON,
-        log::LevelFilter::Info,
-        Some("proxmox-mail-forward"),
-    ) {
+    if let Err(err) = proxmox_log::init_logger("PROXMOX_LOG", LevelFilter::INFO) {
         eprintln!("unable to initialize syslog: {err}");
     }
 
@@ -129,19 +127,19 @@ fn main() {
             if Path::new(PVE_CFG_PATH).exists() {
                 has_pve = true;
                 if let Err(err) = forward_for_pve(&mail) {
-                    log::error!("could not forward mail for Proxmox VE: {err}");
+                    error!("could not forward mail for Proxmox VE: {err}");
                 }
             }
 
             // Assume a PBS installation if /etc/proxmox-backup exists
             if Path::new(PBS_CFG_PATH).exists() {
                 if let Err(err) = forward_for_pbs(&mail, has_pve) {
-                    log::error!("could not forward mail for Proxmox Backup Server: {err}");
+                    error!("could not forward mail for Proxmox Backup Server: {err}");
                 }
             }
         }
         Err(err) => {
-            log::error!("could not read mail from STDIN: {err}")
+            error!("could not read mail from STDIN: {err}")
         }
     }
 }
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH proxmox 1/2] notify: smtp: add missing 'tracing' namespace prefix

[Gabriel Goller]

On 06.12.2024 11:10, Lukas Wagner wrote:
>This section of code is only compiled when the 'mail-forwarder' feature
>is enabled, which might have been the reason why this was missed when the
>other places where log messages are produced were migrated to 'tracing'.

Oh that's why I didn't get an error when compiling.
Anyway consider:

Reviewed-by: Gabriel Goller <g.goller@proxmox.com>

>
>Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
>---
> proxmox-notify/src/endpoints/smtp.rs | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/proxmox-notify/src/endpoints/smtp.rs b/proxmox-notify/src/endpoints/smtp.rs
>index 6bb2d2d0..86911f9b 100644
>--- a/proxmox-notify/src/endpoints/smtp.rs
>+++ b/proxmox-notify/src/endpoints/smtp.rs
>@@ -336,7 +336,7 @@ impl Endpoint for SmtpEndpoint {
>                                 let header = HeaderValue::new(name, value);
>                                 message.headers_mut().insert_raw(header);
>                             }
>-                            Err(e) => error!("could not set header: {e}"),
>+                            Err(e) => tracing::error!("could not set header: {e}"),
>                         }
>                     }
>                 }
>-- 
>2.39.5
>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH proxmox-mail-forward 2/2] switch to proxmox-log

[Gabriel Goller]

> fn main() {
>-    if let Err(err) = syslog::init(
>-        syslog::Facility::LOG_DAEMON,
>-        log::LevelFilter::Info,
>-        Some("proxmox-mail-forward"),
>-    ) {
>+    if let Err(err) = proxmox_log::init_logger("PROXMOX_LOG", LevelFilter::INFO) {
>         eprintln!("unable to initialize syslog: {err}");
>     }
>

Haven't looked at this crate yet, so I don't know where it's all used,
but IMO the new `init_perlmod_logger` would be better? Because the
`init_logger` function will always print to journald and check if a
pbs task exists (which will never happen here afaiu).
We can also add another simpler subscriber which will only log to
journald (I can also do that, just let me know).

Btw now I release how stupid these function names are, I should rename
them in the future eliding the `perlmod`, `cli` names. Something like
`init_journald_and_tasks` and `init_stderr_and_journald`.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH proxmox-mail-forward 2/2] switch to proxmox-log

[Lukas Wagner]

On  2024-12-06 11:40, Gabriel Goller wrote:
>> fn main() {
>> -    if let Err(err) = syslog::init(
>> -        syslog::Facility::LOG_DAEMON,
>> -        log::LevelFilter::Info,
>> -        Some("proxmox-mail-forward"),
>> -    ) {
>> +    if let Err(err) = proxmox_log::init_logger("PROXMOX_LOG", LevelFilter::INFO) {
>>         eprintln!("unable to initialize syslog: {err}");
>>     }
>>
> 
> Haven't looked at this crate yet, so I don't know where it's all used,
> but IMO the new `init_perlmod_logger` would be better? Because the
> `init_logger` function will always print to journald and check if a
> pbs task exists (which will never happen here afaiu).
> We can also add another simpler subscriber which will only log to
> journald (I can also do that, just let me know).

For context, this crate produces the 'proxmox-mail-forward' helper. 
This binary is invoked by the local Postfix daemon in case an email is sent to the
local root user. The executable receives the mail via stdin and feeds it into our
notification stack.

So in this case we only really need need to be logging to journald. I see no harm
in writing logs to stderr as well though, so I think we don't necessarily need to
add another subscriber. As far as I know, Postfix just redirects the output to /dev/null.
> 
> Btw now I release how stupid these function names are, I should rename
> them in the future eliding the `perlmod`, `cli` names. Something like
> `init_journald_and_tasks` and `init_stderr_and_journald`.

That would indeed be a bit more clear :D If you do that now, at least for the perlmod
function, I would rebase this commit onto yours.

Thanks!

-- 
- Lukas



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel