Merge ~robert-ancell/ubuntu/+source/systemd:etc-writable-timezone into ~ubuntu-core-dev/ubuntu/+source/systemd:ubuntu-kinetic

[Robert Ancell]

[Nick Rosbrook (enr0n)]

I have a few questions, because LP: #1981042 does not provide any detail beyond the existing background of why systemd is patched to workaround Ubuntu Core's /etc/writable:

1. What version of systemd is this being observed in? The patches reference the 249.11 source from jammy, but you have proposed it into kinetc.

2. If the intended target really is kinetic, do we really need to bring this into the devel release? Has there been any progress on fixing Ubuntu Core [1] so that we can stop carrying these patches in systemd?

3. If the intended target is jammy, it would need an SRU and a strong justification.

[1] https://bugs.launchpad.net/snappy/+bug/1953172

review: Needs Information

[Robert Ancell (robert-ancell)]

1. Observed in all versions of systemd, as systemd assumes that /etc/locatime is a writable location.

2. Proposing to bring it into kinetic so it can be SRUd to jammy, which is the version we are using to build core based desktop images with. I wasn't aware of any progress on a better solution, but I'll contact Valentin about the bug you linked against. Note the linked git branch only seems to consolidate these patches, it doesn't have any new solution AFAICT.

3. See above.

[Nick Rosbrook (enr0n)]

Understood. I will add rls tags to the bug so that we can discuss in the next Foundations meeting.

[Robert Ancell (robert-ancell)]

Change was merged in https://code.launchpad.net/~enr0n/ubuntu/+source/systemd/+git/systemd/+merge/427302

Unmerged commits

c9bd437... by Robert Ancell on 2022-07-08

Fix timezone not working on read-only /etc (e.g. Ubuntu Core and system-image-based systems)

diff --git a/debian/changelog b/debian/changelog
index fce5cca..dd15247 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,12 @@
+systemd (251.2-2ubuntu2) kinetic; urgency=medium
+
+  * debian/patches/debian/UBUNTU-firstboot-timezone.patch:
+  * debian/patches/debian/UBUNTU-inotify-timezone.patch:
+    - Fix timezone not working on read-only /etc (e.g. Ubuntu Core and
+      system-image-based systems) (LP: #1981042)
+
+ -- Robert Ancell <[e-mail]>  Fri, 08 Jul 2022 12:35:44 +1200
+
 systemd (251.2-2ubuntu1) kinetic; urgency=medium
 
   [ Nick Rosbrook ]
diff --git a/debian/patches/debian/UBUNTU-firstboot-timezone.patch b/debian/patches/debian/UBUNTU-firstboot-timezone.patch
 new file mode 100644
index 0000000..cec5cf9
--- /dev/null
+++ b/debian/patches/debian/UBUNTU-firstboot-timezone.patch
@@ -0,0 +1,55 @@
+Description: On Ubuntu Core /etc/localtime is a symlink, so it needs to be
+ resolved first before finding the file it is linked to.
+Index: systemd-249.11/src/firstboot/firstboot.c
+===================================================================
+--- systemd-249.11.orig/src/firstboot/firstboot.c
++++ systemd-249.11/src/firstboot/firstboot.c
+@@ -459,18 +459,37 @@ static int prompt_timezone(void) {
+         return 0;
+ }
+ 
++/* Hack for Ubuntu Phone/Core: 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;
++}
++
+ static int process_timezone(void) {
+         const char *etc_localtime, *e;
+         int r;
+ 
+-        etc_localtime = prefix_roota(arg_root, "/etc/localtime");
++        etc_localtime = prefix_roota(arg_root, writable_filename("/etc/localtime"));
+         if (laccess(etc_localtime, F_OK) >= 0 && !arg_force)
+                 return 0;
+ 
+         if (arg_copy_timezone && arg_root) {
+                 _cleanup_free_ char *p = NULL;
+ 
+-                r = readlink_malloc("/etc/localtime", &p);
++                r = readlink_malloc(writable_filename("/etc/localtime"), &p);
+                 if (r != -ENOENT) {
+                         if (r < 0)
+                                 return log_error_errno(r, "Failed to read host timezone: %m");
+@@ -491,7 +510,7 @@ static int process_timezone(void) {
+         if (isempty(arg_timezone))
+                 return 0;
+ 
+-        e = strjoina("../usr/share/zoneinfo/", arg_timezone);
++        e = strjoina("/usr/share/zoneinfo/", arg_timezone);
+ 
+         (void) mkdir_parents(etc_localtime, 0755);
+         if (symlink(e, etc_localtime) < 0)
diff --git a/debian/patches/debian/UBUNTU-inotify-timezone.patch b/debian/patches/debian/UBUNTU-inotify-timezone.patch
 new file mode 100644
index 0000000..dccfb2f
--- /dev/null
+++ b/debian/patches/debian/UBUNTU-inotify-timezone.patch
@@ -0,0 +1,50 @@
+Description: On Ubuntu Core /etc/localtime is a symlink, so it needs to be
+ resolved to perform the inotify correctly.
+Index: systemd-249.11/src/core/manager.c
+===================================================================
+--- systemd-249.11.orig/src/core/manager.c
++++ systemd-249.11/src/core/manager.c
+@@ -400,6 +400,25 @@ static int manager_setup_time_change(Man
+         return 0;
+ }
+ 
++/* Hack for Ubuntu Phone/Core: 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;
++}
++
+ static int manager_read_timezone_stat(Manager *m) {
+         struct stat st;
+         bool changed;
+@@ -407,7 +426,7 @@ static int manager_read_timezone_stat(Ma
+         assert(m);
+ 
+         /* Read the current stat() data of /etc/localtime so that we detect changes */
+-        if (lstat("/etc/localtime", &st) < 0) {
++        if (lstat(writable_filename("/etc/localtime"), &st) < 0) {
+                 log_debug_errno(errno, "Failed to stat /etc/localtime, ignoring: %m");
+                 changed = m->etc_localtime_accessible;
+                 m->etc_localtime_accessible = false;
+@@ -444,7 +463,7 @@ static int manager_setup_timezone_change
+          * Note that we create the new event source first here, before releasing the old one. This should optimize
+          * behaviour as this way sd-event can reuse the old watch in case the inode didn't change. */
+ 
+-        r = sd_event_add_inotify(m->event, &new_event, "/etc/localtime",
++        r = sd_event_add_inotify(m->event, &new_event, writable_filename("/etc/localtime"),
+                                  IN_ATTRIB|IN_MOVE_SELF|IN_CLOSE_WRITE|IN_DONT_FOLLOW, manager_dispatch_timezone_change, m);
+         if (r == -ENOENT) {
+                 /* If the file doesn't exist yet, subscribe to /etc instead, and wait until it is created either by
diff --git a/debian/patches/series b/debian/patches/series
index 503bef3..0faee82 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -29,6 +29,8 @@ 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-inotify-timezone.patch
+debian/UBUNTU-firstboot-timezone.patch
 debian/UBUNTU-Revert-namespace-be-more-careful-when-handling-namespacin.patch
 debian/UBUNTU-Revert-cgroup-Continue-unit-reset-if-cgroup-is-busy.patch
 debian/UBUNTU-resolved-default-no-negative-caching.patch