[PATCH v8 4/5] Implement testing framework for pidns

[Ákos Uzonyi]

On Sat, 15 Aug 2020 at 22:27, Dmitry V. Levin <ldv at altlinux.org> wrote:
> On Sun, Aug 09, 2020 at 04:13:05PM +0200, Ákos Uzonyi wrote:
> [...]
> > --- a/tests/init.sh
> > +++ b/tests/init.sh
> > @@ -387,6 +387,37 @@ test_prog_set()
> >       test_pure_prog_set "$@" < "$srcdir/$NAME.in"
> >  }
> >
> > +test_pidns_run_strace()
> > +{
> > +     local parent_pid
> > +
> > +     check_prog tail
> > +     check_prog cut
> > +     check_prog grep
> > +
> > +     run_prog > /dev/null
> > +     run_strace --pidns-translation -f $@ $args > "$EXP"
> > +
> > +     # filter out logs made by the parent or init process of the pidns test
> > +     parent_pid="$(tail -n 2 $LOG | head -n 1 | cut -d' ' -f1)"
> > +     init_pid="$(tail -n 1 $LOG | cut -d' ' -f1)"
> > +     grep -E -v "^($parent_pid|$init_pid) " "$LOG" > "$OUT"
> > +     match_diff "$OUT" "$EXP"
> > +}
>
> init_pid also has to be declared as local here.

RIght, thanks.

> > +
> > +test_pidns()
> > +{
> > +     # ioctl(NS_GET_PARENT) is added in Linux 4.9
> > +     require_min_kernel_version_or_skip 4.9
>
> Is this check still needed given that check_ns_ioctl() already performs
> a runtime check?

I think I left this check because the runtime check seemed to be just
a temporary solution (until the kernel is fixed). But since it's not
expected to happen very soon, I agree that this check should be
removed.

>
> [...]
> > +             if (read(strace_ids_pipe[0], pidns_strace_ids,
> > +                 sizeof(pidns_strace_ids)) < 0)
> > +                     perror_msg_and_fail("read");
>
> If this read call is expected to return sizeof(pidns_strace_ids),
> should the return value be tested against that instead?
>
> [...]
> > +     if (write(strace_ids_pipe[1], pidns_strace_ids,
> > +         sizeof(pidns_strace_ids)) < 0)
> > +             perror_msg_and_fail("write");
>
> Likewise, if this write call is expected to return sizeof(pidns_strace_ids),
> should the return value be tested against that instead?

That makes sense. I'll test against sizeof(pidns_strace_ids).

> > +static void
> > +create_init_process(void)
> > +{
> > +     int child_pipe[2];
> > +     if (pipe(child_pipe) < 0)
> > +             perror_msg_and_fail("pipe");
> > +
> > +     pid_t pid = fork();
> > +     if (pid < 0)
> > +             perror_msg_and_fail("fork");
> > +
> > +     if (!pid) {
> > +             close(child_pipe[1]);
> > +             _exit(read(child_pipe[0], &child_pipe[1], sizeof(int)) == 0);
> > +     }
> > +
> > +     close(child_pipe[0]);
> > +}
>
> If nothing is going to be written to child_pipe[1], then this read call is
> expected to return 0, followed by _exit(1).  The choice of exit code seems
> strange to me.

Oh yes, this is a mistake, I should have written "!=" instead of "==".
I think I'll use an if, that's more readable.

> > +
> > +void
> > +check_ns_ioctl(void)
> > +{
> > +     int fd = open("/proc/self/ns/pid", O_RDONLY);
> > +     if (fd < 0)
> > +             perror_msg_and_fail("opening /proc/self/ns/pid");
>
> What if /proc/self/ns/pid is not available, e.g. /proc is not mounted
> or the kernel doesn't implement it?
> I suggest to do perror_msg_and_skip instead, at least in case of ENOENT.

OK, I'll check for ENOENT