Add syslog supports

[tisonkun]

This refers to #413.

Hello world works locally:

2024-11-12 19:19:52.264557+0800 0x27405d0  Default     0x0                  27738  0    test-aae182a0654f64e3: Hello, nix!

I'd like to send this patch early for any necessary alignments.

[tisonkun]

@SteveLauC I'd like to learn the following things:

1. How are the feature flags structured in this crate?
2. What is the convention to convert type (integers, strings) across the language boundary?
3. How do you typically test a wrapper?
4. Is it OK that I support syslog for macos only for now? I have only macos at hand recently, and cannot programming without really test them out.

[SteveLauC]

Hi, thanks for the PR!

How are the feature flags structured in this crate?

I am not quite sure I understand the question, you mean how to pick a feature? Putting these interfaces under the feature syslog feature looks good

What is the convention to convert type (integers, strings) across the language boundary?

Do you mean the conversion between the Nix wrapper types and the C types used by the libc crate?

• For bitflags, we use the libc_bitflag! macro, the LogFlag type proposed in this PR looks good at first glance.
• For enum, libc_enum! will be used. Types Severity and Facility fall into this category.
• If a C string is needed (string with a tailing NUL), trait NixPath can be used to fill the gap. You can use it for the void openlog(const char *ident argument.
• The priority argument of syslog() and vsyslog() should be constructed by ORing enum Severity and enum Facility, we can probably impl std::ops::BitOr<Output = c_int> for these 2 types, but this is not that fault-resilient, perhaps we should define another wrapper type here🤔
• Regarding the format void syslog(int priority, const char *format, ...); argument, do you think we should implement syslog as a macro similar to println!?

How do you typically test a wrapper?

A few tests to ensure the wrapper would work should be sufficient.

Is it OK that I support syslog for macos only for now?

Yeah, it is ok.

[SteveLauC]

BTW, a changelog is needed, please see CONTRIBUTING.md on how to add one.

[tisonkun]

@SteveLauC Thanks for your information! All updated.

For enum, libc_enum! will be used. Types Severity and Facility fall into this category.

Updated.

If a C string is needed (string with a tailing NUL), trait NixPath can be used to fill the gap. You can use it for the void openlog(const char *ident argument.

Updated.

The priority argument of syslog() and vsyslog() should be constructed by ORing enum Severity and enum Facility, we can probably impl std::ops::BitOr<Output = c_int> for these 2 types, but this is not that fault-resilient, perhaps we should define another wrapper type here🤔

Reasonable. Priority struct added.

Regarding the format void syslog(int priority, const char *format, ...); argument, do you think we should implement syslog as a macro similar to println!?

No. As stated at #413 (comment), I think we just simply use Rust's formatting function and pass the formatted string. This is also how Python wraps syslog functionalities.

A few tests to ensure the wrapper would work should be sufficient.

Included.

[tisonkun]

This patch doesn't wrap vsyslog and setlogmark.

For vsyslog, it's a variant of syslog for ergonomically coding in C, irrelevant to Rust.

For setlogmark, libc doesn't export LOG_MASK and/or LOG_UPTO, make it hard to wrap reasonable code. I temporarily added the wrapper but later revert it. IMO it's less used and we can revisit this function when there is a real-world necessity.

[tisonkun]

LOG_MASK and LOG_UPTO may be easy to add:

#[macro_export]
macro_rules! LOG_MASK 
{
    ($($arg:tt)*) => (
        (1 << $($arg)*)
    )
}

#[macro_export]
macro_rules! LOG_UPTO 
{
    ($($arg:tt)*) => (
        ((1 << (($($arg)*) + 1)) - 1)
    )
}

But I'd like to wait for this patch getting review and perhaps did it in a followup so that we don't push everything at once.

[tisonkun]

@SteveLauC test on unix and updated.

I have an idea to implement setlogmask, but not sure if we can export libc::c_int or we have other better types:

pub fn setlogmask(maskpri: libc::c_int) -> libc::c_int {
    let mask = unsafe { libc::setlogmask(maskpri) };
    mask
}

pub fn log_mask(pri: Severity) -> libc::c_int {
    1 << (pri as libc::c_int)
}

pub fn log_upto(pri: Severity) -> libc::c_int {
    (1 << ((pri as libc::c_int) + 1)) - 1
}

Also related test ideas - https://stackoverflow.com/questions/56049852/python-2-7-setlogmask0-not-disabling-syslog

[SteveLauC]

This interface should be good for UNIXs other than Linux, since Linux would store the ident pointer as-is (will be used during whole process lifetime), we need to add a separate interface for Linux, and gate this with #[cfg(not(target_os = "linux"))].

[SteveLauC]

And thanks for adding the Option here! I didn't catch it in my first round of review.

[tisonkun]

I'd suggest we remove the ident param for now.

It's not easy to just add Option<&'static str> because it may not end with nul and I don't know how to achieve this.

[tisonkun]

I don't observe the ident on macos and linux, perhaps this is because the allocated string is collected 🤣

[tisonkun]

Maybe Option<&'static CStr>. Let me try.

[SteveLauC]

Nix does not pursue uniform interfaces across OSes, if they are different, let them be different. I suggest keeping the original interface for non-Linux platforms as they are not constrained by this limit and adding a separate interface for Linux

[tisonkun]

Could you send a patch to this PR or just edit this PR with the suggested change?

[SteveLauC]

Could you send a patch to this PR

Yeah, let me send a commit

[SteveLauC]

See 596280b

[AlexTMjugador]

I just stumbled upon this review comment when writing a portable interface that relies on openlog and I found the different signature among different Unix flavors quite strange, and potentially inappropriate.

openlog is a function provided by the environment C library, which may be implemented in different ways even for the same Unix flavor (Linux targets routinely use both musl and glibc as C library implementations, and macOS has an history of changing relevant implementation details of this function over OS versions), so changing a function signature just because the target is Linux is something that should be done with lots of caution, as it's likely to not be proper.

Moreover, the POSIX standard says that "the application shall ensure that the string pointed to by ident remains valid during the syslog() calls that will prepend this identifier". Because we can't really know in this function at what times syslog() will be further called, the only safe lifetime for the ident parameter is 'static. And because the string is assumed to be a standard C string with no NUL characters in the middle, it should be CStr, or a str that uses the Nix path helper to check for absence of NULs (more ergonomic). Thus, I think the Linux signature of this function, potentially with a change from CStr to str + Nix path checks, is the one that should be used for all Unix flavors.

What do you think about this? Would a PR to tweak this be welcome? 😄

[tisonkun]

@SteveLauC test on unix and updated.

I have an idea to implement setlogmask, but not sure if we can export libc::c_int or we have other better types:

pub fn setlogmask(maskpri: libc::c_int) -> libc::c_int {
    let mask = unsafe { libc::setlogmask(maskpri) };
    mask
}

pub fn log_mask(pri: Severity) -> libc::c_int {
    1 << (pri as libc::c_int)
}

pub fn log_upto(pri: Severity) -> libc::c_int {
    (1 << ((pri as libc::c_int) + 1)) - 1
}

Also related test ideas - https://stackoverflow.com/questions/56049852/python-2-7-setlogmask0-not-disabling-syslog

@SteveLauC Here is an open question on whether to include setlogmask in this PR.

[SteveLauC]

Regarding the setlogmask() interface, I think we can implement it in the following way:

/// System log priority mask.
#[repr(transparent)]
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct LogMask(libc::c_int);

/// Set the process-wide priority mask to `mask` and return the previous mask 
/// value. 
/// 
/// Calls to `syslog()` with a priority level not set in `mask` are ignored. The 
/// default is to log all priorities.
/// 
/// If the `mask` argument is `None`, the current logmask is not modified, this
/// can be used to query the current log mask.
pub fn setlogmask(mask: Option<LogMask>) -> LogMask {
    let mask = match mask {
        Some(mask) => mask.0,
        None => 0,
    };
    
    let prev_mask = unsafe {
        libc::setlogmask(mask)
    };
    
    LogMask(prev_mask)
}

impl LogMask {
    /// Creates a mask of all priorities up to and including `priority`.
    #[doc(alias("LOG_UPTO"))]
    pub fn up_to(priority: Severity) -> Self {
        todo!()
    }
    
    /// Creates a mask for the specified priority.
    #[doc(alias("LOG_MASK"))]
    pub fn of_priority(priority: Severity)  -> Self {
        todo!()
    }
    
    /// Returns if the mask for the specified `priority` is set.
    pub fn contains(&self, priority: Severity) -> bool {
        todo!()
    }
}

impl std::ops::BitOr for LogMask {
    type Output = Self;
    fn bitor(self, rhs: Self) -> Self::Output {
        Self(self.0 | rhs.0)
    }
}

Though I think the C macros LOG_MASK and LOG_UPTO should be added to the libc crate first, so perhaps we can include these things in the next PR?

[tisonkun]

Though I think the C macros LOG_MASK and LOG_UPTO should be added to the libc crate first, so perhaps we can include these things in the next PR?

Yes. Let's review this PR as is now.

@SteveLauC Thanks for your following commits!

[SteveLauC]

Looks good, thanks for the PR, let's merge it! 🚀