[PATCH] Implement decoding of NS_* ioctl commands

[Dmitry V. Levin]

On Mon, Apr 10, 2017 at 08:21:48AM +0700, Nikolay Marchuk wrote:
> * configure.ac (AC_CHECK_HEADERS): Add linux/nsfs.h.
> * defs.h (DECL_IOCTL(nsfs)): New prototype.
> * ioctl.c (ioctl_decode): Call nsfs_ioctl for 0xb7 code.
> * nsfs.c: New file.
> * nsfs.h: Likewise.
> * Makefile.am (strace_SOURCES): Add them.
> * tests/.gitignore: Add ioctl_nsfs.
> * tests/Makefile.am (check_PROGRAMS): Likewise.
> (DECODER_TESTS): Add ioctl_nsfs.test.
> * tests/ioctl_nsfs.c: New file.
> * tests/ioctl_nsfs.test: Likewise.

style: I prefer "new file" listed before .gitignore update.

[...]
> +#include <linux/ioctl.h>
> +#include "nsfs.h"

Let's move #include <linux/ioctl.h> into nsfs.h, it naturally belongs
there.

> +#include "xlat/setns_types.h"

As setns_types is a static array object, including it in more than one
translation unit results to more than one copy of this array in strace
executable.

Like other xlat arrays used by several translation units,
setns_types should be made a global object by declaring it in defs.h.

[...]
> --- /dev/null
> +++ b/nsfs.h
> @@ -0,0 +1,19 @@
> +#ifndef STRACE_NSFS_H
> +#define STRACE_NSFS_H
> +
> +#ifdef HAVE_LINUX_NSFS_H
> +# include <linux/nsfs.h>
> +#else

Let's #include <linux/ioctl.h> here.

> +# define NSIO    0xb7
> +# define NS_GET_USERNS   _IO(NSIO, 0x1)
> +# define NS_GET_PARENT   _IO(NSIO, 0x2)
> +#endif
[...]
> +#include "tests.h"
> +
> +#include <sched.h>

If CLONE_NEWUSER is not defined, this test won't compile.

[...]
> +static void
> +test_clone(pid_t pid)
> +{
> +	int ns_fd, userns_fd, parent_ns_fd, nstype, rc;
> +	char path[sizeof("/proc/%d/ns/user") + sizeof(int)*3];

If you moved definitions of automatic variables closer to their first use,
the would be slightly more readable.

[...]
> +static int
> +child(void *arg)
> +{
> +	int *pipefd = (int *) arg;
> +	close(pipefd[1]);
> +	/* Wait for EOF from pipe. */
> +	while(read(pipefd[0], &pipefd[1], 1) != 0);

What if read returns -1 somehow?  Let's replace it with

	if (read(pipefd[0], &pipefd[1], 1))
		perror_msg_and_fail("read");

[...]
> +	rc = pipe(pipefd);
> +	if (rc == -1)
> +		perror_msg_and_skip("pipe");

Let's treat it as a fatal error.

> +
> +	pid = clone(child, tail_alloc(1) + 1, (CLONE_NEWUSER | CLONE_UNTRACED
> +	            | SIGCHLD), pipefd);
> +	if (pid != -1) {

Let's print some meaningful diagnostics if clone fails.

> +		close(pipefd[0]);
> +		test_clone(pid);
> +		close(pipefd[1]);
> +		pid = wait(&status);

The good style is to check wait's return code and the status it returns.

[...]
> +match_diff "$OUT" "$EXP"
> +
> +rm -f "$EXP" "$OUT"

You can omit this rm invocation, it's no longer needed.


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20170411/c6a89681/attachment.bin>