Re: [PATCH] timens: show clock symbolic names in /proc/pid/timens_offsets

[Michael Kerrisk (man-pages)]

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/