Re: [systemd-devel] [PATCH] timedated: gather timezone from /etc/localtime sym target
Le mercredi 08 août 2012 à 18:51 +0200, Lennart Poettering a écrit : On Tue, 07.08.12 00:31, Shawn Landen (shawnland...@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. Hmm, I am not entirely sure this is really the best thing to do. Always requiring a symlink for /etc/localtime breaks a couple of things: we can't just bind mount things over in an nspawn container, embedded devices have to ship /usr/share/zoneinfo/, which is probably something they might want to avoid. So, dunno, I think this is something to think about first, discuss the pros and cons. I see that just having this as symlink is much simpler, no doubt, but does it have more benefits? And possibly more disadvantages? Well, I just found a major disavantage: openSUSE (and YaST) are not using a symlink for /etc/localtime but a copy of the zoneinfo file (it is probably coming from support of separate /usr, when mounting /usr wasn't handled by initrd). So, this must be fixed to support /etc/localtime being a copy, at least for reading informations (currently, the code crashes, I'll see if I can fix it). -- Frederic Crozat fcro...@suse.com SUSE ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] timedated: gather timezone from /etc/localtime sym target
Le mardi 14 août 2012 à 11:20 +0200, Frederic Crozat a écrit : Le mercredi 08 août 2012 à 18:51 +0200, Lennart Poettering a écrit : On Tue, 07.08.12 00:31, Shawn Landen (shawnland...@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. Hmm, I am not entirely sure this is really the best thing to do. Always requiring a symlink for /etc/localtime breaks a couple of things: we can't just bind mount things over in an nspawn container, embedded devices have to ship /usr/share/zoneinfo/, which is probably something they might want to avoid. So, dunno, I think this is something to think about first, discuss the pros and cons. I see that just having this as symlink is much simpler, no doubt, but does it have more benefits? And possibly more disadvantages? Well, I just found a major disavantage: openSUSE (and YaST) are not using a symlink for /etc/localtime but a copy of the zoneinfo file (it is probably coming from support of separate /usr, when mounting /usr wasn't handled by initrd). So, this must be fixed to support /etc/localtime being a copy, at least for reading informations (currently, the code crashes, I'll see if I can fix it). After discussing with other SUSE fellows (bnc#773491), I'd say we should just ignore when /etc/localtime is a hardlink (or a copy) to a timezone data (since we don't have the name of the timezone in the file itself) and use /etc/sysconfig/clock as fallback (when available). And when updating timezone, /etc/localtime should be created as a symlink. -- Frederic Crozat fcro...@suse.com SUSE ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] timedated: gather timezone from /etc/localtime sym target
On Tue, 2012-08-14 at 11:20 +0200, Frederic Crozat wrote: Le mercredi 08 août 2012 à 18:51 +0200, Lennart Poettering a écrit : On Tue, 07.08.12 00:31, Shawn Landen (shawnland...@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. Hmm, I am not entirely sure this is really the best thing to do. Always requiring a symlink for /etc/localtime breaks a couple of things: we can't just bind mount things over in an nspawn container, embedded devices have to ship /usr/share/zoneinfo/, which is probably something they might want to avoid. So, dunno, I think this is something to think about first, discuss the pros and cons. I see that just having this as symlink is much simpler, no doubt, but does it have more benefits? And possibly more disadvantages? Well, I just found a major disavantage: openSUSE (and YaST) are not using a symlink for /etc/localtime but a copy of the zoneinfo file (it is probably coming from support of separate /usr, when mounting /usr wasn't handled by initrd). So, this must be fixed to support /etc/localtime being a copy, at least for reading informations (currently, the code crashes, I'll see if I can fix it). This is because its trying to free() the initialized pointer t, after readlink() returns EINVAL. It just has to be initialized to NULL. I'll post a revised 2/3 in a sec. Otherwise, its fallowing your recommendations. It is creating /etc/localtime as an *absolute* too. -- -Shawn Landden ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] timedated: gather timezone from /etc/localtime sym target
Op 8 aug. 2012, om 18:51 heeft Lennart Poettering lenn...@poettering.net het volgende geschreven: On Tue, 07.08.12 00:31, Shawn Landen (shawnland...@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. Hmm, I am not entirely sure this is really the best thing to do. Always requiring a symlink for /etc/localtime breaks a couple of things: we can't just bind mount things over in an nspawn container, embedded devices have to ship /usr/share/zoneinfo/, which is probably something they might want to avoid. For the embedded systems that need TZ support I ship a subset of timezones, it's not that much data. If you're worried about the TZ sizes you shouldn't enable TZ support :) regards, Koen ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] timedated: gather timezone from /etc/localtime sym target
On Tue, 07.08.12 00:31, Shawn Landen (shawnland...@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.
Hmm, I am not entirely sure this is really the best thing to do. Always
requiring a symlink for /etc/localtime breaks a couple of things: we
can't just bind mount things over in an nspawn container, embedded
devices have to ship /usr/share/zoneinfo/, which is probably something
they might want to avoid.
So, dunno, I think this is something to think about first, discuss the
pros and cons. I see that just having this as symlink is much simpler,
no doubt, but does it have more benefits? And possibly more
disadvantages?
Lennart
---
src/timedate/timedated.c | 28
1 file changed, 28 insertions(+)
diff --git a/src/timedate/timedated.c b/src/timedate/timedated.c
index 09fd808..456e409 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
@@ -174,9 +175,35 @@ 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 /usr/share/zoneinfo/);
+} else {
+char *t;
+
+r = readlink_malloc(/etc/localtime, t);
+if (r 0) {
+log_warning(Failed to get target of %s: %m,
/etc/localtime);
+} else if (!startswith(t, /usr/share/zoneinfo/)) {
+log_warning(/etc/localtime should be an absolute
symlink to a timezone data file in /usr/share/zoneinfo/);
+} else {
+tz.zone = strdup(t + strlen(/usr/share/zoneinfo/));
+free(t);
+if (!tz.zone)
+return log_oom();
+
+goto have_timezone;
+}
+
+free(t);
+}
+
r = read_one_line_file(/etc/timezone, tz.zone);
if (r 0) {
if (r != -ENOENT)
@@ -192,6 +219,7 @@ static int read_data(void) {
#endif
}
+have_timezone:
if (isempty(tz.zone)) {
free(tz.zone);
tz.zone = NULL;
Lennart
--
Lennart Poettering - Red Hat, Inc.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] timedated: gather timezone from /etc/localtime sym target
On Wed, Aug 8, 2012 at 6:51 PM, Lennart Poettering lenn...@poettering.net wrote: On Tue, 07.08.12 00:31, Shawn Landen (shawnland...@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. Hmm, I am not entirely sure this is really the best thing to do. Always requiring a symlink for /etc/localtime breaks a couple of things: we can't just bind mount things over in an nspawn container, Just mount the symlink pointing to /usr/share/zoneinfo/ in the container, or possibly to a copy of localtime stored the container's /run, if there isn't anything in the container's zoneinfo? embedded devices have to ship /usr/share/zoneinfo/, which is probably something they might want to avoid. I wouldn't see much of a problem if we can't determine the time zone text in exotic and custom setups. If we don't have stuff available to link to from /etc, we just leave it alone. So, dunno, I think this is something to think about first, discuss the pros and cons. I see that just having this as symlink is much simpler, no doubt, but does it have more benefits? And possibly more disadvantages? Stuff in /etc invites to manual edit and administration. Two files, with very different names, describing the very same thing, which need to be in sync, are so bad, that in the end, there is no real question here, I guess. :) Kay ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] timedated: gather timezone from /etc/localtime sym target
On Wed, Aug 8, 2012 at 6:51 PM, Lennart Poettering lenn...@poettering.net wrote: So, dunno, I think this is something to think about first, discuss the pros and cons. I see that just having this as symlink is much simpler, no doubt, but does it have more benefits? And possibly more disadvantages? I'd very much like /etc/timezone to go away. Mainly due to two reasons: /etc/localtime and /etc/timezone are likely to get out-of-sync (we have the same problem now in Arch where the timezone is stored in /etc/rc.conf rather than in /etc/timezone and it brings us nothing but grief). Also, if /etc/localtime is not a symlink we would need to update it every time the timezone package is updated, this would of course be possible, but it seems unnecessary. -t ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] timedated: gather timezone from /etc/localtime sym target
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 {
+char *t;
+
+r = readlink_malloc(/etc/localtime, t);
+if (r 0) {
+log_warning(Failed to get target of %s: %m,
/etc/localtime);
+} else if (!startswith(t, ZONEINFO_PATH)) {
+log_warning(/etc/localtime should be an absolute
symlink to a timezone data file in %s, ZONEINFO_PATH);
+} else {
+tz.zone = strdup(t + strlen(ZONEINFO_PATH));
+if (!tz.zone) {
+free(t);
+return log_oom();
+}
+
+free(t);
+goto have_timeone;
+}
+
+free(t);
+}
+
r = read_one_line_file(/etc/timezone, tz.zone);
if (r 0) {
if (r != -ENOENT)
@@ -192,6 +225,7 @@ static int read_data(void) {
#endif
}
+have_timezone:
if (isempty(tz.zone)) {
free(tz.zone);
tz.zone = NULL;
--
1.7.10.4
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] timedated: gather timezone from /etc/localtime sym target
On Wed, 08.08.12 19:29, Shawn Landen (shawnland...@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.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] timedated: gather timezone from /etc/localtime sym target
From: Shawn Landen shawnland...@gmail.com
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.
---
src/timedate/timedated.c | 30 ++
1 file changed, 30 insertions(+)
diff --git a/src/timedate/timedated.c b/src/timedate/timedated.c
index 09fd808..d0812fa 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,11 @@
BUS_GENERIC_INTERFACES_LIST \
org.freedesktop.timedate1\0
+/* Must start and end with '/' */
+#ifndef ZONEINFO_PATH
+# define ZONEINFO_PATH /usr/share/zoneinfo/
+#endif
+
const char timedate_interface[] _introspect_(timedate1) = INTERFACE;
typedef struct TZ {
@@ -174,9 +180,32 @@ static void verify_timezone(void) {
static int read_data(void) {
int r;
+struct stat st;
+char *t;
free_data();
+r = readlink_malloc(/etc/localtime, t);
+if (r 0) {
+if (errno == EINVAL)
+log_warning(/etc/localtime should be an absolute
symlink to a timezone data file in %s, ZONEINFO_PATH);
+else
+log_warning(Failed to get target of %s: %m,
/etc/localtime);
+} else if (!startswith(t, ZONEINFO_PATH))
+log_warning(/etc/localtime should be an absolute symlink to a
timezone data file in %s, ZONEINFO_PATH);
+else {
+tz.zone = strdup(t + strlen(ZONEINFO_PATH));
+if (!tz.zone) {
+free(t);
+return log_oom();
+}
+
+free(t);
+goto have_timeone;
+}
+
+free(t);
+
r = read_one_line_file(/etc/timezone, tz.zone);
if (r 0) {
if (r != -ENOENT)
@@ -192,6 +221,7 @@ static int read_data(void) {
#endif
}
+have_timezone:
if (isempty(tz.zone)) {
free(tz.zone);
tz.zone = NULL;
--
1.7.10.4
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] timedated: gather timezone from /etc/localtime sym target
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
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] timedated: gather timezone from /etc/localtime sym target
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.
---
src/timedate/timedated.c | 28
1 file changed, 28 insertions(+)
diff --git a/src/timedate/timedated.c b/src/timedate/timedated.c
index 09fd808..456e409 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
@@ -174,9 +175,35 @@ 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 /usr/share/zoneinfo/);
+} else {
+char *t;
+
+r = readlink_malloc(/etc/localtime, t);
+if (r 0) {
+log_warning(Failed to get target of %s: %m,
/etc/localtime);
+} else if (!startswith(t, /usr/share/zoneinfo/)) {
+log_warning(/etc/localtime should be an absolute
symlink to a timezone data file in /usr/share/zoneinfo/);
+} else {
+tz.zone = strdup(t + strlen(/usr/share/zoneinfo/));
+free(t);
+if (!tz.zone)
+return log_oom();
+
+goto have_timezone;
+}
+
+free(t);
+}
+
r = read_one_line_file(/etc/timezone, tz.zone);
if (r 0) {
if (r != -ENOENT)
@@ -192,6 +219,7 @@ static int read_data(void) {
#endif
}
+have_timezone:
if (isempty(tz.zone)) {
free(tz.zone);
tz.zone = NULL;
--
1.7.10.4
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
