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 <mtk.manpages@xxxxxxxxx>
Tested-by: Michael Kerrisk <mtk.manpages@xxxxxxxxx>
But, I do have one small suggestion below.
> Fixes: 04a8682a71be ("fs/proc: Introduce /proc/pid/timens_offsets")
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
> Cc: Michael Kerrisk <mtk.manpages@xxxxxxxxx>
> Cc: Dmitry Safonov <0x7f454c46@xxxxxxxxx>
> Suggested-by: Michael Kerrisk <mtk.manpages@xxxxxxxxx>
> Signed-off-by: Andrei Vagin <avagin@xxxxxxxxx>
> ---
> 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/
