[systemd-devel] Various bug fixes

[Lennart Poettering]

On Fri, 21.09.12 15:25, Václav Pavlín (vpavlin at redhat.com) wrote:

> You can find few patches for various bugs in attachement.

Thanks!

I'll let Kay merge the udev related patches. The rest I merged, with a
few exceptions:

> +        r = 0;
>          if (!isempty(cvtnr))
> -                safe_atou32(cvtnr, &vtnr);
> +                r = safe_atou32(cvtnr, &vtnr);
>  
> -        if (!isempty(display) && vtnr <= 0) {
> +        if (!isempty(display) && !r && vtnr <= 0) {
>                  if (isempty(seat))
>                          get_seat_from_display(display, &seat, &vtnr);
>                  else if (streq(seat, "seat0"))

vtnr here is actually initialized to zero anyway, so if safe_atou32()
fails it will still be 0, and we'll detect this. With other words: the
missing error checking is not actually missing, I simply folded the
parse error check into the range check... To clarify this I have now
commited a patch explaining this.

> -        if (mount("/etc/localtime", where, "bind", MS_BIND, NULL) >= 0)
> -                mount("/etc/localtime", where, "bind", MS_BIND|MS_REMOUNT|MS_RDONLY, NULL);
> +        if (mount("/etc/localtime", where, "bind", MS_BIND, NULL) >= 0) {
> +                if (mount("/etc/localtime", where, "bind", MS_BIND|MS_REMOUNT|MS_RDONLY, NULL) < 0)
> +                        log_warning("mount(%s) failed, file will be writable: %m", where);
> +        } else {
> +                log_error("mount(%s) failed: %m", where);
> +                return -errno;
> +        }

Not sure about this. /etc/localtime mounting is really not that
important. Basically we do it here just to be nice, which is why I am
not generating any error messages for it. 

That all said this entire function is now broken, and should be fixed,
as /etc/localtime is now a symlink and we cannot bind mount
symlinks. I'll change it to simply sync the symlink by creating it anew.


> -        if (mount("/etc/resolv.conf", where, "bind", MS_BIND, NULL) >= 0)
> -                mount("/etc/resolv.conf", where, "bind", MS_BIND|MS_REMOUNT|MS_RDONLY, NULL);
> +        if (mount("/etc/resolv.conf", where, "bind", MS_BIND, NULL) >= 0) {
> +                if (mount("/etc/resolv.conf", where, "bind", MS_BIND|MS_REMOUNT|MS_RDONLY, NULL) < 0)
> +                        log_warning("mount(%s) failed, file will be writable: %m", where);
> +        } else {
> +                log_error("mount(%s) failed: %m", where);
> +                return -errno;
> +        }

Overmounting /etc/resolv.conf is another one of these cases where's it's
just nice to do this, but doesn't really matter, which is why we fail silently.

> +++ b/src/shared/hwclock.c
> @@ -78,7 +78,7 @@ static int rtc_open(int flags) {
>                  p = strjoin("/sys/class/rtc/", de->d_name, "/hctosys", NULL);
>                  if (!p) {
>                          closedir(d);
> -                        return -ENOMEM;
> +                        return log_oom();
>                  }

Hmm, so, I don't think we should invoke log_oom() here. "Library-style" calls
like this one should probably not generate messages on their own, but
leave that to their callers. Returning ENOMEM is hence the right thing
here.

log_oom() is something for actual program code. The lines between
"library code" and "program code" are a bit blurry, admittedly, and
there are a couple of occasion where this currently not handled
correctly in the codebase, but we still should try to follow this rule
wherever applicable.

>                  r = read_one_line_file(p, &v);
> @@ -94,6 +94,12 @@ static int rtc_open(int flags) {
>                          continue;
>  
>                  p = strappend("/dev/", de->d_name);
> +
> +                if (!p) {
> +                        closedir(d);
> +                        return log_oom();
> +                }

This part is important however. so I canged this to -ENOMEM and commited
it.

> >From 4f9408af71ff55aeac3b51d2fe6208c1cb8e2417 Mon Sep 17 00:00:00 2001
> From: Lukas Nykryn <lnykryn at redhat.com>
> Date: Fri, 21 Sep 2012 12:59:15 +0200
> Subject: [PATCH 16/18] journal: free JournalFile when *ret is null

Hmm, we shouldn't allow passing in a NULL ret here anyway. I have now
changed the sources to ensure this.

Thanks a ton for the thorough review! Much appreciated!

Lennart

-- 
Lennart Poettering - Red Hat, Inc.