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
signature.asc
Description: PGP signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Strace-devel mailing list Strace-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/strace-devel
