[systemd-devel] [PATCH] Avoid a busy systemd-journald in LXC environments

[Lennart Poettering]

On Mon, 02.12.13 16:27, Werner Fink (werner at suse.de) wrote:

> that is the systemd-journald may ignore /dec/kmsg which are not a valid
> device but a lint to /dev/null as done in LinuX Containers (LXC).  This
> change adds straight forward a check for /dev/kmsg.
> 
> Signed-off-by: Werner Fink <werner at suse.de>

We don't use "S-o-b" in systemd...

>  #include <systemd/sd-messages.h>
>  #include <libudev.h>
> @@ -377,12 +379,32 @@ int server_flush_dev_kmsg(Server *s) {
>  
>  int server_open_dev_kmsg(Server *s) {
>          struct epoll_event ev;
> +        struct stat st;
>  
>          assert(s);
>  
>          s->dev_kmsg_fd = open("/dev/kmsg", O_RDWR|O_CLOEXEC|O_NONBLOCK|O_NOCTTY);
>          if (s->dev_kmsg_fd < 0) {
> -                log_warning("Failed to open /dev/kmsg, ignoring: %m");
> +                /* Do not warn as it may not exists in LXC environments */
> +                if (errno != ENOENT)
> +                        log_warning("Failed to open /dev/kmsg, ignoring: %m");
> +                return 0;
> +        }

This part makes sense though I'd prefer if we'd just downgrade the
message here on ENOENT, not completely get rid of.

    log_full(errno == ENOENT ? LOG_DEBUG : LOG_WARNING, ...

or something like that...

> +
> +        if (fstat(s->dev_kmsg_fd, &st) < 0) {
> +                log_error("Failed to stat /dev/kmsg fd, ignoring: %m");
> +                close_nointr_nofail(s->dev_kmsg_fd);
> +                s->dev_kmsg_fd = -1;
> +                return 0;
> +        }
> +
> +        if (!S_ISCHR(st.st_mode) || major(st.st_rdev) != 1 || minor(st.st_rdev) != 11) {
> +                int old_errno = errno;
> +                errno = ENODEV;
> +                log_warning("Irregular device /dev/kmsg, ignoring: %m");
> +                errno = old_errno;
> +                close_nointr_nofail(s->dev_kmsg_fd);
> +                s->dev_kmsg_fd = -1;

I am really not convinced by this. LXC should either not set up
/dev/kmsg, or should do it in a way that doesn't create problems with
what userspace expects. I am pretty sure the onus here is on LXC to
provide something that is compatible or nothing at all...

Also, we do not hardcode major/minor pairs. Ever.

>                  return 0;
>          }
>  
> @@ -391,6 +413,9 @@ int server_open_dev_kmsg(Server *s) {
>          ev.data.fd = s->dev_kmsg_fd;
>          if (epoll_ctl(s->epoll_fd, EPOLL_CTL_ADD, s->dev_kmsg_fd, &ev) < 0) {
>  
> +                close_nointr_nofail(s->dev_kmsg_fd);
> +                s->dev_kmsg_fd = -1;

This part looks OK.

> +
>                  /* This will fail with EPERM on older kernels where
>                   * /dev/kmsg is not readable. */
>                  if (errno == EPERM)

Could you rework your patch to just inclide the first bit (downgradingof
the log message to LOG_DEBUG if errno == ENOENT) and the last bit? I'd
merge it then.

Lennart

-- 
Lennart Poettering, Red Hat