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

Proposed by Robert Ancell
Status: Rejected
Rejected by: Robert Ancell
Proposed branch: ~robert-ancell/ubuntu/+source/systemd:etc-writable-timezone
Merge into: ~ubuntu-core-dev/ubuntu/+source/systemd:ubuntu-kinetic
Diff against target: 147 lines (+116/-0)
4 files modified
debian/changelog (+9/-0)
debian/patches/debian/UBUNTU-firstboot-timezone.patch (+55/-0)
debian/patches/debian/UBUNTU-inotify-timezone.patch (+50/-0)
debian/patches/series (+2/-0)
Reviewer Review Type Date Requested Status
Nick Rosbrook Needs Information
Review via email: mp+426518@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Nick Rosbrook (enr0n) wrote :

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
Revision history for this message
Robert Ancell (robert-ancell) wrote :

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.

Revision history for this message
Nick Rosbrook (enr0n) wrote :

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

Revision history for this message
Robert Ancell (robert-ancell) wrote :

Unmerged commits

c9bd437... by Robert Ancell

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index fce5cca..dd15247 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,12 @@
6+systemd (251.2-2ubuntu2) kinetic; urgency=medium
7+
8+ * debian/patches/debian/UBUNTU-firstboot-timezone.patch:
9+ * debian/patches/debian/UBUNTU-inotify-timezone.patch:
10+ - Fix timezone not working on read-only /etc (e.g. Ubuntu Core and
11+ system-image-based systems) (LP: #1981042)
12+
13+ -- Robert Ancell <robert.ancell@canonical.com> Fri, 08 Jul 2022 12:35:44 +1200
14+
15 systemd (251.2-2ubuntu1) kinetic; urgency=medium
16
17 [ Nick Rosbrook ]
18diff --git a/debian/patches/debian/UBUNTU-firstboot-timezone.patch b/debian/patches/debian/UBUNTU-firstboot-timezone.patch
19new file mode 100644
20index 0000000..cec5cf9
21--- /dev/null
22+++ b/debian/patches/debian/UBUNTU-firstboot-timezone.patch
23@@ -0,0 +1,55 @@
24+Description: On Ubuntu Core /etc/localtime is a symlink, so it needs to be
25+ resolved first before finding the file it is linked to.
26+Index: systemd-249.11/src/firstboot/firstboot.c
27+===================================================================
28+--- systemd-249.11.orig/src/firstboot/firstboot.c
29++++ systemd-249.11/src/firstboot/firstboot.c
30+@@ -459,18 +459,37 @@ static int prompt_timezone(void) {
31+ return 0;
32+ }
33+
34++/* Hack for Ubuntu Phone/Core: check if path is an existing symlink to
35++ * /etc/writable; if it is, update that instead */
36++static const char* writable_filename(const char *path) {
37++ ssize_t r;
38++ static char realfile_buf[PATH_MAX];
39++ _cleanup_free_ char *realfile = NULL;
40++ const char *result = path;
41++ int orig_errno = errno;
42++
43++ r = readlink_and_make_absolute(path, &realfile);
44++ if (r >= 0 && startswith(realfile, "/etc/writable")) {
45++ snprintf(realfile_buf, sizeof(realfile_buf), "%s", realfile);
46++ result = realfile_buf;
47++ }
48++
49++ errno = orig_errno;
50++ return result;
51++}
52++
53+ static int process_timezone(void) {
54+ const char *etc_localtime, *e;
55+ int r;
56+
57+- etc_localtime = prefix_roota(arg_root, "/etc/localtime");
58++ etc_localtime = prefix_roota(arg_root, writable_filename("/etc/localtime"));
59+ if (laccess(etc_localtime, F_OK) >= 0 && !arg_force)
60+ return 0;
61+
62+ if (arg_copy_timezone && arg_root) {
63+ _cleanup_free_ char *p = NULL;
64+
65+- r = readlink_malloc("/etc/localtime", &p);
66++ r = readlink_malloc(writable_filename("/etc/localtime"), &p);
67+ if (r != -ENOENT) {
68+ if (r < 0)
69+ return log_error_errno(r, "Failed to read host timezone: %m");
70+@@ -491,7 +510,7 @@ static int process_timezone(void) {
71+ if (isempty(arg_timezone))
72+ return 0;
73+
74+- e = strjoina("../usr/share/zoneinfo/", arg_timezone);
75++ e = strjoina("/usr/share/zoneinfo/", arg_timezone);
76+
77+ (void) mkdir_parents(etc_localtime, 0755);
78+ if (symlink(e, etc_localtime) < 0)
79diff --git a/debian/patches/debian/UBUNTU-inotify-timezone.patch b/debian/patches/debian/UBUNTU-inotify-timezone.patch
80new file mode 100644
81index 0000000..dccfb2f
82--- /dev/null
83+++ b/debian/patches/debian/UBUNTU-inotify-timezone.patch
84@@ -0,0 +1,50 @@
85+Description: On Ubuntu Core /etc/localtime is a symlink, so it needs to be
86+ resolved to perform the inotify correctly.
87+Index: systemd-249.11/src/core/manager.c
88+===================================================================
89+--- systemd-249.11.orig/src/core/manager.c
90++++ systemd-249.11/src/core/manager.c
91+@@ -400,6 +400,25 @@ static int manager_setup_time_change(Man
92+ return 0;
93+ }
94+
95++/* Hack for Ubuntu Phone/Core: check if path is an existing symlink to
96++ * /etc/writable; if it is, update that instead */
97++static const char* writable_filename(const char *path) {
98++ ssize_t r;
99++ static char realfile_buf[PATH_MAX];
100++ _cleanup_free_ char *realfile = NULL;
101++ const char *result = path;
102++ int orig_errno = errno;
103++
104++ r = readlink_and_make_absolute(path, &realfile);
105++ if (r >= 0 && startswith(realfile, "/etc/writable")) {
106++ snprintf(realfile_buf, sizeof(realfile_buf), "%s", realfile);
107++ result = realfile_buf;
108++ }
109++
110++ errno = orig_errno;
111++ return result;
112++}
113++
114+ static int manager_read_timezone_stat(Manager *m) {
115+ struct stat st;
116+ bool changed;
117+@@ -407,7 +426,7 @@ static int manager_read_timezone_stat(Ma
118+ assert(m);
119+
120+ /* Read the current stat() data of /etc/localtime so that we detect changes */
121+- if (lstat("/etc/localtime", &st) < 0) {
122++ if (lstat(writable_filename("/etc/localtime"), &st) < 0) {
123+ log_debug_errno(errno, "Failed to stat /etc/localtime, ignoring: %m");
124+ changed = m->etc_localtime_accessible;
125+ m->etc_localtime_accessible = false;
126+@@ -444,7 +463,7 @@ static int manager_setup_timezone_change
127+ * Note that we create the new event source first here, before releasing the old one. This should optimize
128+ * behaviour as this way sd-event can reuse the old watch in case the inode didn't change. */
129+
130+- r = sd_event_add_inotify(m->event, &new_event, "/etc/localtime",
131++ r = sd_event_add_inotify(m->event, &new_event, writable_filename("/etc/localtime"),
132+ IN_ATTRIB|IN_MOVE_SELF|IN_CLOSE_WRITE|IN_DONT_FOLLOW, manager_dispatch_timezone_change, m);
133+ if (r == -ENOENT) {
134+ /* If the file doesn't exist yet, subscribe to /etc instead, and wait until it is created either by
135diff --git a/debian/patches/series b/debian/patches/series
136index 503bef3..0faee82 100644
137--- a/debian/patches/series
138+++ b/debian/patches/series
139@@ -29,6 +29,8 @@ debian/UBUNTU-test-sleep-skip-test_fiemap-upon-inapproriate-ioctl-.patch
140 debian/UBUNTU-Support-system-image-read-only-etc.patch
141 debian/timedatectl-lp1650688.patch
142 debian/UBUNTU-Fix-timezone-setting-on-read-only-etc.patch
143+debian/UBUNTU-inotify-timezone.patch
144+debian/UBUNTU-firstboot-timezone.patch
145 debian/UBUNTU-Revert-namespace-be-more-careful-when-handling-namespacin.patch
146 debian/UBUNTU-Revert-cgroup-Continue-unit-reset-if-cgroup-is-busy.patch
147 debian/UBUNTU-resolved-default-no-negative-caching.patch

Subscribers

People subscribed via source and target branches