[systemd-devel] [RFC] systemd syslogd

[Lennart Poettering]

On Thu, 16.06.11 22:35, William Douglas (william.r.douglas at gmail.com) wrote:

(Read my reply to Bill's mail first)

> -	units/syslog-bridge.target
> +	units/syslog-bridge.target \

We probably indeed should rename the two logging related services in
systemd. Right now "systemd-logger" connects stdout/stderr of a service
with syslog, and "systemd-kmsg-syslogd" connects /dev/log temporarily
with kmsg. We probably should simplify and streamline that to
"systemd-stdio-syslog-bridge" and "systemd-syslog-kmsg-bridge" or so.

> +# See systemd-syslog.conf(5) for details
> +
> +[kern]
> +Filename=/var/log/kern
> +Facility=kern

I like the simplicity of this. But how do you route multiple facilities
into the same file?

Also, I am tempted to say that facilities for msg routing are a bit too
simple and probably deserve replacing one day. More specifically what
I'd like to see is a kernel patch that introduces SCM_CGROUPS or so
which makes it possible to send information about cgroup membership of a
process over a socket in a trusted way (similar to
SCM_CREDENTIALS). This would allow us route logging based on service
names (which cannot be faked) which I think is much nicer and more
flexible than the 12 standard facilities (of which at least two are
mostly obsolete anyway, i.e. UUCP and FTP).

> +struct SyslogSection {
> +        /* mutex should be held whenever changes are being made to the log file
> +           or when fsize is being updated */
> +        pthread_mutex_t file_mtx;
> +        long long fsize;

File sizes on Unix are off_t.

> +        char *section;
> +        char *filename;
> +        unsigned int facility;
> +        unsigned int creation_perms;
> +        int priority;
> +        char rotate_type;
> +        int compress;
> +        int log_count;
> +        union {
> +                time_t time;
> +                long long size;

Here too.

> +static time_t first_rotate_time(void) {
> +        Iterator it;
> +        SyslogSection *sys = NULL;
> +        time_t shortest, left;
> +        struct stat st;
> +        struct timeval tv;
> +
> +        shortest = ~(shortest & 0);
> +        shortest ^= 1 << (sizeof(time_t) * 8 - 1);
> +
> +        if (gettimeofday(&tv, NULL) < 0)
> +                return DEFAULT_SYSLOGD_RTIMER;

In systemd I generally try to avoid gettimeofday in favour clock_gettime().

> +
> +static void rotate_time_handler(int sig, siginfo_t *si, void *uc);
> +
> +static int setup_log_rotate(void) {
> +        timer_t timerid;
> +        struct sigevent sev;
> +        struct itimerspec its;
> +        struct sigaction sa;
> +
> +        sa.sa_sigaction = rotate_time_handler;
> +        sigemptyset(&sa.sa_mask);
> +
> +        its.it_value.tv_sec = first_rotate_time();
> +        if (sigaction(SIGUSR1, &sa, NULL) == -1) {
> +                log_error("Couldn't set rotate signal action");
> +                return -1;
> +        }
> +        sev.sigev_signo = SIGUSR1;
> +
> +        sev.sigev_notify = SIGEV_SIGNAL;
> +        sev.sigev_value.sival_ptr = &timerid;
> +        if (timer_create(CLOCK_MONOTONIC, &sev, &timerid) == -1) {
> +                log_error("Couldn't create timer");
> +                return -1;
> +        }
> +
> +        if (timer_settime(timerid, 0, &its, NULL) == -1) {
> +                log_error("Couldn't set timer");
> +                return -1;
> +        }
> +
> +        return 0;
> +}
> +

Urks. Signals! I'd strongly recommend using signalfd() and timerfd() as
the only interfaces to signalling and timing to ever use.


> +
> +        for (i = 0; i < LOG_NFACILITIES; i++) {
> +                if (facility_count[i] > 0) {
> +                        sys_list = new(SyslogSection*,
> facility_count[i]+1);

The code in systemd is generally OOM safe, and I think a syslog
implementation should probably be too, so that OOM messages can be
properly collected. i.e. i's probably wise to check for return values
here.

> +                        for (j = 0; j < facility_count[i]+1; j++)
> +                                sys_list[j] = NULL;

In systemd the macro new0() (instead of new()) can be used to initialize
to zero. when allocating.

> +                } else
> +                        sys_list = NULL;
> +
> +                syslog_conf[i] = sys_list;
> +        }
> +
> +        if (insert_to_syslog_conf(facility_count))
> +                return -1;
> +
> +        return 0;
> +}
> +

In general I must say that I actually like the code (and the coding
style) very much, so it's hard for me to say no to this.

KUTGW,

Lennart

-- 
Lennart Poettering - Red Hat, Inc.