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

[Lennart Poettering]

On Wed, 08.08.12 19:29, Shawn Landen (shawnlandden at gmail.com) wrote:

> keep other method for now, consider dropping later.
> 
> Supporting relative links here could be problematic as timezones in
> /usr/share/zoneinfo are often themselves symlinks (and symlinks to
> symlinks), so this implamentation only only support absolute links.
> 
> v2 - Add ZONEINFO_PATH
>    - Restructured the patch so its very straight forward to remove
> support for the old methods. (some log_warning()s should then be converted
> to log_error() too)
> ---
>  src/timedate/timedated.c |   34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/src/timedate/timedated.c b/src/timedate/timedated.c
> index 09fd808..01d01b6 100644
> --- a/src/timedate/timedated.c
> +++ b/src/timedate/timedated.c
> @@ -24,6 +24,7 @@
>  #include <errno.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include <sys/stat.h>
>  
>  #include "util.h"
>  #include "strv.h"
> @@ -74,6 +75,10 @@
>          BUS_GENERIC_INTERFACES_LIST             \
>          "org.freedesktop.timedate1\0"
>  
> +#ifndef ZONEINFO_PATH
> +#  define ZONEINFO_PATH "/usr/share/zoneinfo/"
> +#endif
> +
>  const char timedate_interface[] _introspect_("timedate1") = INTERFACE;
>  
>  typedef struct TZ {
> @@ -174,9 +179,37 @@ static void verify_timezone(void) {
>  
>  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.

> +                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"...

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...

Hmm, I think it would make sense to write /etc/timezone only if it
already exists.

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?

Lennart

-- 
Lennart Poettering - Red Hat, Inc.