Bug 14888 - Qt5 QTimeZone::systemTimeZoneId() returns broken value - timezone set broken installer
Summary: Qt5 QTimeZone::systemTimeZoneId() returns broken value - timezone set broken ...
Status: RESOLVED FIXED
Alias: None
Product: Mageia
Classification: Unclassified
Component: RPM Packages (show other bugs)
Version: Cauldron
Hardware: x86_64 Linux
Priority: Normal normal
Target Milestone: ---
Assignee: Thierry Vignaud
QA Contact:
URL:
Whiteboard:
Keywords:
: 15301 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-12-25 21:11 CET by Ãlo Parri
Modified: 2015-06-01 09:18 CEST (History)
9 users (show)

See Also:
Source RPM:
CVE:
Status comment:


Attachments
patch to unlink and symlink new localtime (706 bytes, patch)
2015-01-04 21:07 CET, Angelo Naselli
Details | Diff
possible patch (805 bytes, patch)
2015-02-15 11:41 CET, Angelo Naselli
Details | Diff

Description Ãlo Parri 2014-12-25 21:11:40 CET
qDebug() << QTimeZone::systemTimeZoneId();

output:
Test(11843)/default main: "urope/Tallin"

expected output:
Test(11843)/default main: "Europe/Tallinn"

Also the broken value gets written to ~/.config/ktimezonedrc and wrong time is shown in system try on plasma 5 desktop.

It has probably been so for a while now, maybe 3 months.

Reproducible: 

Steps to Reproduce:
Sander Lepik 2014-12-25 21:20:48 CET

CC: (none) => mageia, mageia

Comment 1 Jüri Ivask 2015-01-01 20:28:28 CET
Found that /etc/localtime was now a binary file.
Just as a experiment I renamed it to localtime.old and created a symlink named localtime pointing to /usr/share/zoneinfo/Europe/Tallinn and the clock in Plasma 5 system try started to show correct time at once

CC: (none) => jyri2000

Comment 2 Davide Nifosi 2015-01-03 14:36:33 CET
The problem seems to be in Qt5 qtimezoneprivate_tz.cpp: systemTimeZoneId() tries to obtain the system time zone in several ways, and it seems for Mageia (or at least for me) it ends up parsing /etc/sysconfig/clock, but it parses the value wrong

Line 943:

if (line.left(5) == QStringLiteral("ZONE=")) {
    ianaId = line.mid(6, line.size() - 7).toUtf8();
} ...

I suppose it should be

ianaId = line.mid(5, line.size() - 5).toUtf8();

and have no idea why it's done like that instead.

CC: (none) => ita84

Comment 3 Davide Nifosi 2015-01-03 16:19:22 CET
Nevermind, it does that because in RHEL /etc/sysconfig/clock values are in double quotes (e.g ZONE="Europe/Berlin"); in Mageia there are no quotes.

Anyway, it's an upstream issue, I believe, since none of the other ways systemTimeZoneId() uses to determine the system time zone work either: they're as follows:

- using the TZ environmental variable
- parsing /etc/timezone (Debian)
- extracting zone name from the /etc/localtime symlink, if applicable
- parsing /etc/sysconfig/clock, and assuming RHEL format with quotes
- if all else fails, set to UTC

I guess a workaround would be to set the TZ variable; I don't really see a clean way for Mageia to patch around this.
Comment 4 Jüri Ivask 2015-01-03 16:58:12 CET
So is it that the Mageia installer issue, that here, instead of creating a /etc/localtime symlink to /usr/share/zoneinfo/continent/city file it copies that city file to /etc and names it to localtime?
Comment 5 Davide Nifosi 2015-01-04 09:01:48 CET
I don't know what choices were made by Mageia regarding timezone settings, but having a copy of the timezone file in /etc/localtime instead of a symlink seems to be a legitimate configuration (I believe Gentoo uses it, probably because of the chance of a separate /usr partition; in that case, though, the timezone string is found in /etc/timezone instead, and that's accounted for in QTimeZone).

On the other hand, I believe that systemd's timedatectl assumes /etc/localtime is a symlink, so maybe doing that would be a better alternative, assuming we don't want to screw over users with a separate /usr partition.

Again, I have no idea why things are like they are in Mageia now, so I don't know if any changes to the timezone configuration could affect other parts of the system.
Sander Lepik 2015-01-04 11:47:36 CET

CC: (none) => mageia

Comment 6 Colin Guthrie 2015-01-04 16:37:33 CET
I really think we should just ensure that /etc/localtime is a symlink. Other things rely on this too.

The only possible argument would be if /etc is on a separate partition to /usr and /usr was mounted late but this really shouldn't affect us in Mageia (/usr is mounted early in initrd).

So yeah, myself and Angelo noticed this the other day too for other things.

I think Angelo was working on making our tools not write a symlink. Can you confirm?

CC: (none) => anaselli

Comment 7 Angelo Naselli 2015-01-04 18:18:33 CET
@Colin well i have a patch for the mana tools, that it's easy to port to drakxclock, i.e. unlink localtime and link it with pointing to the new file.
Tell me if you want me to commit it, we can then move the discussion on dev mailing list if something is not wrong or can be implemented better
Comment 8 Sander Lepik 2015-01-04 18:27:03 CET
Angelo, you can port it and attach, so Colin can review it :)
Comment 9 Angelo Naselli 2015-01-04 21:07:08 CET
Created attachment 5784 [details]
patch to unlink and symlink new localtime

This patch should work, but i cannot test it atm. I will add the similar one to mana tools if this works.
Comment 10 Sander Lepik 2015-01-22 10:06:27 CET
Colin, can you review please? So we can get it fixed before release :)
Comment 11 Angelo Naselli 2015-01-22 10:15:03 CET
Before reviewing could you test if using manaclock, the original problem is solved? Or at least linking localtime... Because if it isn't there is no reason in porting this patch but using dbus correctly...
Comment 12 Jüri Ivask 2015-01-22 11:27:53 CET
Made a test:
1. after installation corrected the time by creating sysmlink /etc/localtime pointing to /usr/share/zoneinfo/Europe/Tallinn
2. changed timezone using drakclock to /Europe/Helsinki (actually the same timezone) - clock started to show wrong time - probably UTC (bios time).
3. changed timezone using manaclock back to /Europe/Tallinn - clock started to show correct time according to timezone.

Had to run manaclock as root. Running as user it showed that the imezone was changed while in reality it was'nt.
Comment 13 Angelo Naselli 2015-01-22 11:35:27 CET
> Had to run manaclock as root. Running as user it showed that the imezone was 
> changed while in reality it was'nt.
yes i'm moving lemme lemme to dbus as far as i can so all can be used bu users and
password is promped when needed. In mana clock i probably forgot to check the user id because once user could change the setting as in drakclock... anyway the test seems to work
Comment 14 Angelo Naselli 2015-01-22 17:34:31 CET
Jüri i made some tests and there is no need to use manaclock as root, but to run it properly using "mana clock" or "pkexec /usr/bin/manaclock" is required atm.
Comment 15 Sander Lepik 2015-01-24 13:48:02 CET
Thierry, can you please review the patch as you are the maintainer.

Assignee: bugsquad => thierry.vignaud

Comment 16 Thierry Vignaud 2015-01-24 14:47:35 CET
get rid of unlink() and just use symlinkf().
Also I think we should keep the eval {} :
    eval { symlinkf($tz_prefix . '/' . $t->{timezone}, "$::prefix/etc/localtime") };
Comment 17 Angelo Naselli 2015-01-24 16:42:04 CET
interesting i always thought to eval to trap exceptions that would crash the program more than functions returning 0, is that a case in which eval could work correctly (such as unlink and symlink do)? 

BTW symlinkf is just a subroutine that contains a call to unlink and then one to symlink, so i don't see a real difference there :).
Comment 18 Angelo Naselli 2015-02-04 23:22:51 CET
I tried to use eval without any real benefit, so please explain why i should use it.
A problem in that code is that if the link (or old copy) fails the program goes on anyway in my opinion, despite on 'eval', 'if' or any other checks, it just log atm.
Comment 19 Mageia Robot 2015-02-08 22:36:48 CET
commit f6d83149036c800310fb307b79a477c614141056
Author: Colin Guthrie <colin@...>
Date:   Sun Feb 8 21:33:24 2015 +0000

    drakclock: Ensure that /etc/localtime is a symlink (mga#14888)
    
    This is actually just a followup to d4be53b62 to use the wrapper symlinkf() function
    which seems a little bit neater (subjective) and to update NEWS file.
---
 Commit Link:
   http://gitweb.mageia.org/software/drakx/commit/?id=f6d83149036c800310fb307b79a477c614141056
Comment 20 David Walser 2015-02-10 01:36:57 CET
This commit is going to horribly break things.  /etc/localtime is a copy of the timezone file, as established by the timezone package and the update-localtime script that comes with it.  If we're going to change this to a symlink, it needs to be adapted there.  Otherwise, there will be horrible issues due to the different file types (regular file vs. symlink).

CC: (none) => luigiwalser

Comment 21 Sander Lepik 2015-02-10 09:26:40 CET
Haven't seen any problems yet, and I did that same symlink change on my mga4 system too. If timezone package is doing some other changes then yeah, this package must be fixed too. But this commit is a change in the right direction.
Comment 22 Colin Guthrie 2015-02-10 09:54:29 CET
In fact the whole point of *copying* the zoneinfo file becomes obsolete with a symlink anyway, so that script can die and we can just add a post script to transition to a symlink if it's a file. Should be trivial.
Comment 23 Colin Guthrie 2015-02-10 10:12:44 CET
OK, after Sander's update, I did a one-time transition to a symlink. Does that look OK David? Can you think of any other areas where this would cause problems?
Comment 24 Colin Guthrie 2015-02-10 11:27:29 CET
Actually thinking about this, I think it's better to use triggerun here as we don't really want this to happen all the time - overwriting the current symlink with the potentially stale data in /etc/sysconfig/clock.

e.g. under GNOME, if you have automatic timezone adjustment on, then the symlink will be updated for you (via timedated), and if you upgrade your timezone package, you don't want the (now stale) info in /etc/sysconfig/clock kicking in.

So let's just transition once.
Comment 25 Colin Guthrie 2015-02-10 11:28:43 CET
Gah. That won't actually be needed will it as it's alreayd a symlink then :s Pah. No harm tho' IMO.
Comment 26 David Walser 2015-02-10 12:15:55 CET
Looks OK to me.  We'll have to test it and see how it goes.

Just to be clear, I do think symlinking is better.  Fedora already does this, and I like the self-documenting nature of being able to tell which zone it is by reading the link.
Comment 27 Colin Guthrie 2015-02-10 12:56:23 CET
:)

FWIW, /etc/sysconfig/clock is now pretty much totally obsolete. ZONE= is in the symlink target name, UTC=true/false is encoded in the third line of /etc/adjtime.

I don't know what ARC is and the only reference I can find is in perl-install/timezone.pm which always sets it to false, so think it's pointless. This then tallies up with how timedated does things. So the next step is to kill off the parsing/use of this sysconfig file.
Comment 28 Angelo Naselli 2015-02-13 23:32:48 CET
dbus api seems to work fine: 
http://gitweb.mageia.org/software/adminpanel/tree/lib/AdminPanel/Shared/TimeZone.pm
if our DEs work fine we could close this issue....
Comment 29 Doug Laidlaw 2015-02-15 06:05:30 CET
I don't know if I am on the right bug, but...

After a clean install of Beta 3, my /etc/localtime was set to 

/mnt/usr/share/zoneinfo/Australia/Melbourne

The zone was right, but the /mnt looks like a leftover from the installation process.  An error like this could explain the bug that KDE couldn't set the time, etc.

(I wish there was a quick way of looking up that bug number without leaving this screen.)

CC: (none) => laidlaws

Comment 30 Thierry Vignaud 2015-02-15 09:18:40 CET
Yes, bug was introduced by commit f6d83149036c8003 above
Comment 31 Doug Laidlaw 2015-02-15 10:01:48 CET
The first symptom was that Linux was seeing hardware time as UTC, and Windows was seeing it as local time.  A bit of Googling led me to the fix.
Comment 32 Angelo Naselli 2015-02-15 10:09:17 CET
The commit didn't consider the installer, so after installation you have that wrong side effect. Anyway the KDE problem was present already, since we had no
link before @Doug on comment#31 please try drakclock and see if now the link is correctly set and KDE work right.
Comment 33 Mageia Robot 2015-02-15 10:53:01 CET
commit 46ce1e3284d595bd7be1ab41a79ecd67730553e4
Author: Colin Guthrie <colin@...>
Date:   Sun Feb 15 09:52:03 2015 +0000

    Fix timezone dir install prefix prepending (broken symlink during installer mga#14888)
---
 Commit Link:
   http://gitweb.mageia.org/software/drakx/commit/?id=46ce1e3284d595bd7be1ab41a79ecd67730553e4
Comment 34 Colin Guthrie 2015-02-15 10:54:18 CET
Thierry, does the above look OK? Not sure if the API is called outside of drakx repo (always a problem when changing this code - no idea where else it's used!) but I can't think of too many places which would want this - especially in an installer context!
Comment 35 Doug Laidlaw 2015-02-15 10:58:56 CET
(In reply to Mageia Robot from comment #33)
> commit 46ce1e3284d595bd7be1ab41a79ecd67730553e4
> Author: Colin Guthrie <colin@...>
> Date:   Sun Feb 15 09:52:03 2015 +0000
> 
>     Fix timezone dir install prefix prepending (broken symlink during
> installer mga#14888)
> ---
>  Commit Link:
>   
> http://gitweb.mageia.org/software/drakx/commit/
> ?id=46ce1e3284d595bd7be1ab41a79ecd67730553e4

Yes Sir!

The Robot Apocalypse has started already!
Comment 36 Doug Laidlaw 2015-02-15 11:15:36 CET
(In reply to Angelo Naselli from comment #32)
> The commit didn't consider the installer, so after installation you have
> that wrong side effect. Anyway the KDE problem was present already, since we
> had no
> link before @Doug on comment#31 please try drakclock and see if now the link
> is correctly set and KDE work right.

I re-worked the symlink as it should have been.  Immediately the correct time was shown on GkrellM.  I run Xfce, not KDE.  I logged out and in again, and the correct time showed in the notification area.  I have my clock synchronised with NTP.  In this case setting the time with drakclock did nothing, just exited.  I didn't need to look again.
Comment 37 Thierry Vignaud 2015-02-15 11:34:27 CET
(In reply to Colin Guthrie from comment #34)
> Thierry, does the above look OK? Not sure if the API is called outside of
> drakx repo (always a problem when changing this code - no idea where else
> it's used!) but I can't think of too many places which would want this -
> especially in an installer context!

this looks a little big ugly :-(
I would have extracted a get_raw_timezone_prefix() from get_timezone_prefix() instead...
Comment 38 Angelo Naselli 2015-02-15 11:41:02 CET
Created attachment 5920 [details]
possible patch

Could this patch sound better?
If so i'll commit later at home
Comment 39 Thierry Vignaud 2015-02-15 11:46:38 CET
yes this looks better.
Though we use the following style for options:
  my ($system_prefix) = @_;

Also, for perl_checker, we prefix boolean options with "b_"

So we would rename system_prefix to b_system_prefix.
As it's an hint, I would further rename it to $b_use_system_prefix

Last but not least, in write(), the $system_prefix variable is useless, just use:
    my $tz_prefix = get_timezone_prefix(1);
Comment 40 Thierry Vignaud 2015-02-15 11:46:54 CET
After "git revert" previous commit of course
Comment 41 Angelo Naselli 2015-02-15 12:38:09 CET
On comment#39 yes the get_timezone_prefix($use_system_prefix) was only for readability :)
Comment 42 Mageia Robot 2015-02-15 12:55:38 CET
commit b26a69c7edbec529be9d325414e8cb47ca132182
Author: Angelo Naselli <anaselli@...>
Date:   Sun Feb 15 12:45:58 2015 +0100

    Revert "Fix timezone dir install prefix prepending (broken symlink during installer mga#14888)"
    
    This reverts commit 46ce1e3284d595bd7be1ab41a79ecd67730553e4.
---
 Commit Link:
   http://gitweb.mageia.org/software/drakx/commit/?id=b26a69c7edbec529be9d325414e8cb47ca132182
Comment 43 Mageia Robot 2015-02-15 12:55:40 CET
commit 9ff316b07c005c0e43e2a9f0546baa85a3abaaae
Author: Angelo Naselli <anaselli@...>
Date:   Sun Feb 15 12:54:28 2015 +0100

    Fix timezone dir install prefix prepending (broken symlink during installer mga#14888)
---
 Commit Link:
   http://gitweb.mageia.org/software/drakx/commit/?id=9ff316b07c005c0e43e2a9f0546baa85a3abaaae
Manuel Hiebel 2015-02-16 21:31:49 CET

Summary: Qt5 QTimeZone::systemTimeZoneId() returns broken value => Qt5 QTimeZone::systemTimeZoneId() returns broken value - timezone set broken installer

Comment 44 Manuel Hiebel 2015-02-16 21:33:04 CET
*** Bug 15301 has been marked as a duplicate of this bug. ***

CC: (none) => junknospam

Comment 45 Samuel Verschelde 2015-05-31 23:41:24 CEST
So, is this bug fixed after the latest commits in comment 42 and comment 43?
Comment 46 Doug Laidlaw 2015-06-01 04:10:54 CEST
Not present in a clean install of the RC.
Comment 47 Samuel Verschelde 2015-06-01 09:18:08 CEST
Fixed then.

Status: NEW => RESOLVED
Resolution: (none) => FIXED


Note You need to log in before you can comment on or make changes to this bug.