Re: [PATCH] timens: show clock symbolic names in /proc/pid/timens_offsets
From: Michael Kerrisk (man-pages) <hidden>
Date: 2020-04-11 10:32:52
Also in:
lkml
Hello Andrei, On 4/11/20 8:52 AM, Andrei Vagin wrote:
Michael Kerrisk suggested to replace numeric clock IDs on symbolic names. Now the content of these files looks like this: $ cat /proc/5362/timens_offsets monotonic 9504000 0 boottime 3456000 0 For setting offsets, both representations of clocks can be used. As for compatibility, it is acceptable to change things as long as userspace doesn't care. The format of timens_offsets files is very new and there are no userspace tools that rely on this format. But three projects crun, util-linux and criu rely on the interface of setting time offsets and this is why we need to continue supporting the clock IDs in this case.
Thanks very much for this patch, which I've tested. So: Acked-by: Michael Kerrisk <redacted> Tested-by: Michael Kerrisk <redacted> But, I do have one small suggestion below.
quoted hunk ↗ jump to hunk
Fixes: 04a8682a71be ("fs/proc: Introduce /proc/pid/timens_offsets") Cc: Thomas Gleixner <redacted> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Eric W. Biederman <redacted> Cc: Michael Kerrisk <redacted> Cc: Dmitry Safonov <redacted> Suggested-by: Michael Kerrisk <redacted> Signed-off-by: Andrei Vagin <redacted> --- fs/proc/base.c | 14 +++++++++++++- kernel/time/namespace.c | 15 ++++++++++++++- 2 files changed, 27 insertions(+), 2 deletions(-)diff --git a/fs/proc/base.c b/fs/proc/base.c index 6042b646ab27..572898dd16a0 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c@@ -1573,6 +1573,7 @@ static ssize_t timens_offsets_write(struct file *file, const char __user *buf, noffsets = 0; for (pos = kbuf; pos; pos = next_line) { struct proc_timens_offset *off = &offsets[noffsets]; + char clock[10]; int err; /* Find the end of line and ensure we don't look past it */@@ -1584,10 +1585,21 @@ static ssize_t timens_offsets_write(struct file *file, const char __user *buf, next_line = NULL; } - err = sscanf(pos, "%u %lld %lu", &off->clockid, + err = sscanf(pos, "%9s %lld %lu", clock, &off->val.tv_sec, &off->val.tv_nsec); if (err != 3 || off->val.tv_nsec >= NSEC_PER_SEC) goto out; + + clock[sizeof(clock) - 1] = 0; + if (strcmp(clock, "monotonic") == 0 || + strcmp(clock, __stringify(CLOCK_MONOTONIC)) == 0) + off->clockid = CLOCK_MONOTONIC; + else if (strcmp(clock, "boottime") == 0 || + strcmp(clock, __stringify(CLOCK_BOOTTIME)) == 0) + off->clockid = CLOCK_BOOTTIME; + else + goto out; + noffsets++; if (noffsets == ARRAY_SIZE(offsets)) { if (next_line)diff --git a/kernel/time/namespace.c b/kernel/time/namespace.c index e6ba064ce773..8127d2647064 100644 --- a/kernel/time/namespace.c +++ b/kernel/time/namespace.c@@ -338,7 +338,20 @@ static struct user_namespace *timens_owner(struct ns_common *ns) static void show_offset(struct seq_file *m, int clockid, struct timespec64 *ts) { - seq_printf(m, "%d %lld %ld\n", clockid, ts->tv_sec, ts->tv_nsec); + char *clock; + + switch (clockid) { + case CLOCK_BOOTTIME: + clock = "boottime"; + break; + case CLOCK_MONOTONIC: + clock = "monotonic"; + break; + default: + clock = "unknown"; + break; + }
As things stand, there is to my eye an excessive amount of white space in the output produced by this line:
+ seq_printf(m, "%s\t%10lld\t%10ld\n", clock, ts->tv_sec, ts->tv_nsec);
Can I suggest instead something like: seq_printf(m, "%-16s %10lld %9ld\n", clock, ts->tv_sec, ts->tv_nsec); Thanks, Michael -- Michael Kerrisk Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/ Linux/UNIX System Programming Training: http://man7.org/training/