Merge ~peat-new/ubuntu/+source/systemd:ubuntu-jammy_-_timedated-symlink into ~ubuntu-core-dev/ubuntu/+source/systemd:ubuntu-jammy

[Ratchanan Srirattanamet]

Fix timedated unable to retrieve & properly set timezone on read-only /etc

LP: #1650688
LP: #1733881

[Dan Streetman (ddstreet)]

"Hack for Ubuntu phone"...personally I'm pretty strongly *against* these hacks to systemd. I'd prefer to rip all the hackery out completely from Ubuntu's systemd; Ubuntu core should figure out some way for things to work without having to patch everything.

I'm adding my personal rejection to this, but it certainly isn't up to me.

review: Disapprove (meaningless)

[Alberto Mardegan (mardy)]

Hi Dan! I'm still in the process of investigating this through, but at a first sight Ratchanan's patch appears correct and needed: we already have a similar patch in debian/patches/debian/UBUNTU-Support-system-image-read-only-etc.patch but for some reason (systemd code changed, or some wrong patch refresh?) it is not complete.

The bug, indeed, still exists (I just tested on Ubuntu Core18 in QEMU), and tomorrow I'll try this branch and report back. Most likely we want the first patch to be merged with the existing one which I mentioned before, however.

[Dan Streetman (ddstreet)]

> we already have a similar patch in debian/patches/debian/UBUNTU-Support-system-image-read-only-etc.patch

from 2014. Which IMHO should be dropped. We should not carry hacks for 7 years; something should be changed *correctly*. In this case, it seems like that something is either upstream systemd (unlikely), or ubuntu core needs to figure out how to work without hacking systemd.

> but for some reason (systemd code changed, or some wrong patch refresh?) it is
> not complete.

One of many reasons why this kind of forever-hacking junk is horrible.

[Alberto Mardegan (mardy)]

Hi Dan, let's continue the discussion in https://bugs.launchpad.net/snappy/+bug/1650688 :-)

[Michael Vogt (mvo)]

Fwiw, I asked Valentin from my team to look into a better solution that can be upstreamed. However in the short term we have real customers with real issue because this is missing. I understand the concerns and I also feel uneasy about this (I have ported this or similar patches more than once and did not like it one bit). I would suggest we merge this one to unblock our customer and in parallel work on a proper solution. How does that sound?

[Dan Streetman (ddstreet)]

> However in the short term we have real customers with real issue because this is missing

er, this MR is for jammy. If there are any existing real customers using jammy, that's a problem. ;-)

Fixing this in stable releases is a completely separate issue from hacking the devel release.

As I said, this isn't up to me; I have no formal ownership of systemd for Ubuntu and I don't really do anything for systemd for the devel release. However, IMHO this absolutely shouldn't go into Jammy; some kind of *correct* design of core22 should be done to allow 'stock' systemd to work without hacks. But again, feel free to disregard my opinion and throw it in :)

As far as fixing the stable releases, I'm certainly not thrilled about the approach, but it seems fine to get the hack(s) working correctly if there is a problem with it on core18 or core20 (or core16); I don't see any reason to redesign anything on stable releases.

[Ratchanan Srirattanamet (peat-new)]

> er, this MR is for jammy.

I submitted this for Jammy because, in my understanding, SRU requires that the change be included in the current devel series first.

I've already submitted the MPs for Focal and Bionic as well [1][2]. For us at UBports, the Focal series is the most important, because we're targetting Focal for our big upgrade. So, if it's fixed in Focal, it's Ok for me if this specific MP won't be merged. I'm not sure how Ubuntu's development process works though.

[1] https://code.launchpad.net/~peat-new/ubuntu/+source/systemd/+git/systemd/+merge/409313
[2] https://code.launchpad.net/~peat-new/ubuntu/+source/systemd/+git/systemd/+merge/409314

[Dan Streetman (ddstreet)]

> I submitted this for Jammy because, in my understanding, SRU requires that the change be included in the current devel series first.

right, that's the normal requirement; however the fix used in stable releases doesn't necessarily have to be the same as a fix in the devel relase and/or upstream; the SRU fix can/should be different if the upstream fix is larger or more complex than is needed to fix something in a stable release. For example, if a bug was fixed upstream by a large refactoring/rewrite of some subsystem, it wouldn't be appropriate to backport the rewrite, and would be fine to manually create some patch to fix only a specific problem, even though that patch would not be how the bug was fixed in later releases.

For this particular issue, and again IMHO, this should be fixed *correctly* upstream (i.e. the devel release), and fixed *simply* (i.e. with the patch to hack systemd) in stable releases (though SRUing a "correct" fix would be preferable, if possible).

diff --git a/debian/changelog b/debian/changelog
index 39a517f..0e79a4f 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,5 +1,6 @@
 systemd (248.3-1ubuntu9) UNRELEASED; urgency=medium
 
+  [ Dan Streetman ]
   * d/p/lp1950508-cgroup-check-if-any-controller-is-in-use-as-v1.patch:
     Use cgroupv1 in container if host using cgroupv1
     (LP: #1950508)
@@ -7,6 +8,13 @@ systemd (248.3-1ubuntu9) UNRELEASED; urgency=medium
     New meson doesn't allow bool + bool
     (LP: #1950511)
 
+  [ Ratchanan Srirattanamet ]
+  * d/p/debian/timedatectl-lp1650688.patch,
+    d/p/debian/UBUNTU-Fix-timezone-setting-on-read-only-etc.patch:
+    Fix timedated unable to retrieve & properly set timezone on
+    read-only /etc (e.g. Ubuntu Core and system-image-based systems)
+    (LP: #1650688, LP: #1733881)
+
  -- Dan Streetman <[e-mail]>  Wed, 10 Nov 2021 13:47:28 -0500
 
 systemd (248.3-1ubuntu8) impish; urgency=medium
diff --git a/debian/patches/debian/UBUNTU-Fix-timezone-setting-on-read-only-etc.patch b/debian/patches/debian/UBUNTU-Fix-timezone-setting-on-read-only-etc.patch
new file mode 100644
index 0000000..dccf2e0
--- /dev/null
+++ b/debian/patches/debian/UBUNTU-Fix-timezone-setting-on-read-only-etc.patch
@@ -0,0 +1,28 @@
+Description: Fix timezone setting on read-only etc
+ Due to our read-only /etc workaround, the localtime link on such
+ system ends up in /etc/writable, not /etc. To make the link target
+ correct in both normal and such systems, makes the path absolute.
+ .
+ On Ubuntu Core, this eliminates the need for the wrapper script, and
+ makes the DBus interface work properly.
+Author: Ratchanan Srirattanamet <[e-mail]>
+Origin: other
+Bug-Ubuntu: https://bugs.launchpad.net/snappy/+bug/1650688
+Forwarded: not-needed (part of read-only /etc workaround)
+Last-Update: 2021-09-24
+---
+This patch header follows DEP-3: http://dep.debian.net/deps/dep3/
+--- a/src/timedate/timedated.c
++++ b/src/timedate/timedated.c
+@@ -320,9 +320,9 @@
+                         return r;
+                 }
+ 
+-                source = "../usr/share/zoneinfo/UTC";
++                source = "/usr/share/zoneinfo/UTC";
+         } else {
+-                p = path_join("../usr/share/zoneinfo", c->zone);
++                p = path_join("/usr/share/zoneinfo", c->zone);
+                 if (!p)
+                         return -ENOMEM;
+ 
diff --git a/debian/patches/debian/timedatectl-lp1650688.patch b/debian/patches/debian/timedatectl-lp1650688.patch
new file mode 100644
index 0000000..35bc48c
--- /dev/null
+++ b/debian/patches/debian/timedatectl-lp1650688.patch
@@ -0,0 +1,53 @@
+Description: Fix retrieving timezone on read-only /etc
+ get_timezone() retrieve it by reading the link destination of
+ /etc/localtime, which on systems with read-only /etc will always point
+ to /etc/writable. Makes this function aware of the /etc/writable
+ redirection and handle it.
+ .
+ [[e-mail]: add descrtiption and other metadata.]
+Author: Michael Vogt <[e-mail]>
+Origin: vendor, https://bugs.launchpad.net/snappy/+bug/1650688/comments/46
+Bug-Ubuntu: https://bugs.launchpad.net/snappy/+bug/1650688
+Forwarded: not-needed (part of read-only /etc workaround)
+Last-Update: 2021-09-24
+---
+This patch header follows DEP-3: http://dep.debian.net/deps/dep3/
+diff --git a/src/basic/time-util.c b/src/basic/time-util.c
+index d7028ac..b9bb4da 100644
+--- a/src/basic/time-util.c
++++ b/src/basic/time-util.c
+@@ -1391,6 +1391,25 @@ bool clock_supported(clockid_t clock) {
+         }
+ }
+ 
++/* Hack for Ubuntu phone: check if path is an existing symlink to
++ * /etc/writable; if it is, update that instead */
++static const char* writable_filename(const char *path) {
++        ssize_t r;
++        static char realfile_buf[PATH_MAX];
++        _cleanup_free_ char *realfile = NULL;
++        const char *result = path;
++        int orig_errno = errno;
++
++        r = readlink_and_make_absolute(path, &realfile);
++        if (r >= 0 && startswith(realfile, "/etc/writable")) {
++                snprintf(realfile_buf, sizeof(realfile_buf), "%s", realfile);
++                result = realfile_buf;
++        }
++
++        errno = orig_errno;
++        return result;
++}
++
+ int get_timezone(char **ret) {
+         _cleanup_free_ char *t = NULL;
+         const char *e;
+@@ -1398,7 +1417,7 @@ int get_timezone(char **ret) {
+         int r;
+         bool use_utc_fallback = false;
+ 
+-        r = readlink_malloc("/etc/localtime", &t);
++        r = readlink_malloc(writable_filename("/etc/localtime"), &t);
+         if (r < 0) {
+                 if (r == -ENOENT)
+                         use_utc_fallback = true;
diff --git a/debian/patches/series b/debian/patches/series
index 81330b5..0880584 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -34,6 +34,8 @@ debian/UBUNTU-journald.service-set-Nice-1-to-dodge-watchdog-on-soft-loc.patch
 debian/UBUNTU-units-block-CAP_SYS_MODULE-units-in-containers-too.patch
 debian/UBUNTU-test-sleep-skip-test_fiemap-upon-inapproriate-ioctl-.patch
 debian/UBUNTU-Support-system-image-read-only-etc.patch
+debian/timedatectl-lp1650688.patch
+debian/UBUNTU-Fix-timezone-setting-on-read-only-etc.patch
 debian/UBUNTU-units-disable-journald-watchdog.patch
 debian/UBUNTU-Revert-namespace-be-more-careful-when-handling-namespacin.patch
 debian/UBUNTU-Revert-cgroup-Continue-unit-reset-if-cgroup-is-busy.patch