[systemd-devel] [PATCH] timedated: gather timezone from /etc/localtime sym target

[shawn]

On Wed, 2012-08-08 at 22:56 +0200, Lennart Poettering wrote: 
> >  static int read_data(void) {
> >          int r;
> > +        struct stat st;
> >  
> >          free_data();
> >  
> > +        r = lstat("/etc/localtime", &st);
> > +        if (r < 0) {
> > +                log_warning("lstat() of %s failed: %m", "/etc/localtime");
> > +        } else if (!S_ISLNK(st.st_mode)) {
> > +                log_warning("/etc/localtime should be an absolute symlink to a timezone data file in %s", ZONEINFO_PATH);
> > +        } else {
> 
> Coding style: "if" blocks containing only one line are generally written
> without {} in the systemd codebase.
fixed 
> 
> > +                char *t;
> > +
> > +                r = readlink_malloc("/etc/localtime", &t);
> 
> Please invoke this call right-away, don't bother with the lstat() as
> readlink() will return EINVAL anyway if it isn't a symbolic link what
> you are reading. It's good to have this "as atomic as possible"...
using the EINVAL behavior made it much cleaner, just realized i forgot
to
remove <sys/stat.h>.... 
> 
> Maybe use readlink_and_make_absolute() here? Or even
> readlink_and_canonicalize()? That's the one-stop solution to getting a
> useful file name out of a symlink...
The problem is that canonicalize resolves symlinks inside
of /usr/share/zoneinfo
as is mentioned in the changelog, which ruins the detection of the
timezone.

A hack could be to use
char *o;
...
readlink_and_make_absolute("/etc/localtime", &t);
..
if (!(o = strstr(t, ZONEINFO_PATH)))
...
strdup(o + strlen(ZONEINFO_PATH));

which would make it work for relative symlinks that do not reside
somewhere in /usr,
but seems really nasty over just requiring an absolute symlink. 
> 
> Hmm, I think it would make sense to write /etc/timezone only if it
> already exists.
I'll look into this, but if its doing that this is a differn't bug that
already exists in the current version when built for Fedora. 
> 
> Also, we probably should drop the man page timezone(5) at the same time,
> as we are not pushing that anymore then. But maybe we should add
> localtime(5) instead which explains the semantics of the symlink?
Considering that it must be an absolute symlink in the only sane way I
can think to implement it, and that at least Debian currently ship it as
a normal file, I think that would
be sensible.



Does anyone have a sensible way in which to allow symlinks here? The
complicated ways in which ".." works when directories are themselves
symlinks makes me think this is something too complicated.

-- 
-Shawn Landden