On Fri, 21.09.12 15:25, Václav Pavlín (vpav...@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 <lnyk...@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.
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
