Message ID | xnr2evq358.fsf@greed.delorie.com |
---|---|
State | New |
Headers | show |
Series | test-container: move postclean outside of namespace changes | expand |
On 12/5/18 12:44 PM, DJ Delorie wrote: > Move the postclean step to a separate process that forks just before > we change anything in the namespace, so that when the test exits > we can do the cleanup in the original namespace. I would like to see the commit message for this change include a more detailed description of the problem. Suggest: ~~~ During postclean.req testing it was found that the fork in the parent process (after the unshare syscall) would fail with ENOMEM (see recursive_remove() in test-container.c). While failing with ENOMEM is certainly unexpected, it is simply easier to refactor the design and have the parent remain outside of the namespace. This change moves the postclean.req processing to a distinct process (the parent) that then forks the test process (which will have to fork once more to complete uid/gid transitions). When the test process exists the cleanup process will ensure all files are deleted when a post clean is requested. Signed-off-by: DJ Delorie <dj@redhat.com> Reviewed-by: Carlos O'Donell <carlos@redhat.com> ~~~ OK for master. Tested on x86_64 with tst-localedef-hardlinks, which now passes cleanly with a postclean.req. Reviewed-by: Carlos O'Donell <carlos@redhat.com> > [BZ #23948] > * support/test-container.c: Move postclean step to before we > change namespaces. > > diff --git a/ChangeLog b/ChangeLog > index 4138a08b4e..14c3bb6967 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,3 +1,9 @@ > +2018-12-05 DJ Delorie <dj@redhat.com> > + > + [BZ #23948] > + * support/test-container.c: Move postclean step to before we > + change namespaces. > + OK. > 2018-12-04 Joseph Myers <joseph@codesourcery.com> > > * Makefile ($(objpfx)testroot.pristine/install.stamp): Do not run > diff --git a/support/test-container.c b/support/test-container.c > index df450adfdb..1d1aebeaf3 100644 > --- a/support/test-container.c > +++ b/support/test-container.c > @@ -921,6 +921,43 @@ main (int argc, char **argv) > } > } > > + if (do_postclean) OK. postclean.req is detected. > + { > + pid_t pc_pid = fork (); > + > + if (pc_pid < 0) > + { > + FAIL_EXIT1 ("Can't fork for post-clean"); > + } OK. > + else if (pc_pid > 0) > + { > + /* Parent. */ > + int status; > + waitpid (pc_pid, &status, 0); > + > + /* Child has exited, we can post-clean the test root. */ > + printf("running post-clean rsync\n"); > + rsync (pristine_root_path, new_root_path, 1); OK. passing 1 for and_delete (3rd arg of rsync) so we delete all files that shouldn't be there. > + > + if (WIFEXITED (status)) > + exit (WEXITSTATUS (status)); > + > + if (WIFSIGNALED (status)) > + { > + printf ("%%SIGNALLED%%\n"); > + exit (77); > + } > + > + printf ("%%EXITERROR%%\n"); > + exit (78); > + } > + > + /* Child continues. */ > + } > + > + /* This is the last point in the program where we're still in the > + "normal" namespace. */ OK. > + > #ifdef CLONE_NEWNS > /* The unshare here gives us our own spaces and capabilities. */ > if (unshare (CLONE_NEWUSER | CLONE_NEWPID | CLONE_NEWNS) < 0) > @@ -974,14 +1011,6 @@ main (int argc, char **argv) > int status; > waitpid (child, &status, 0); > > - /* There's a bit of magic here, since the buildroot is mounted > - in our space, the paths are still valid, and since the mounts > - aren't recursive, it sees *only* the built root, not anything > - we would normally se if we rsync'd to "/" like mounted /dev > - files. */ > - if (do_postclean) > - rsync (pristine_root_path, new_root_path, 1); OK. Happy to see any bit of magic removed. > - > if (WIFEXITED (status)) > exit (WEXITSTATUS (status)); > >
diff --git a/ChangeLog b/ChangeLog index 4138a08b4e..14c3bb6967 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2018-12-05 DJ Delorie <dj@redhat.com> + + [BZ #23948] + * support/test-container.c: Move postclean step to before we + change namespaces. + 2018-12-04 Joseph Myers <joseph@codesourcery.com> * Makefile ($(objpfx)testroot.pristine/install.stamp): Do not run diff --git a/support/test-container.c b/support/test-container.c index df450adfdb..1d1aebeaf3 100644 --- a/support/test-container.c +++ b/support/test-container.c @@ -921,6 +921,43 @@ main (int argc, char **argv) } } + if (do_postclean) + { + pid_t pc_pid = fork (); + + if (pc_pid < 0) + { + FAIL_EXIT1 ("Can't fork for post-clean"); + } + else if (pc_pid > 0) + { + /* Parent. */ + int status; + waitpid (pc_pid, &status, 0); + + /* Child has exited, we can post-clean the test root. */ + printf("running post-clean rsync\n"); + rsync (pristine_root_path, new_root_path, 1); + + if (WIFEXITED (status)) + exit (WEXITSTATUS (status)); + + if (WIFSIGNALED (status)) + { + printf ("%%SIGNALLED%%\n"); + exit (77); + } + + printf ("%%EXITERROR%%\n"); + exit (78); + } + + /* Child continues. */ + } + + /* This is the last point in the program where we're still in the + "normal" namespace. */ + #ifdef CLONE_NEWNS /* The unshare here gives us our own spaces and capabilities. */ if (unshare (CLONE_NEWUSER | CLONE_NEWPID | CLONE_NEWNS) < 0) @@ -974,14 +1011,6 @@ main (int argc, char **argv) int status; waitpid (child, &status, 0); - /* There's a bit of magic here, since the buildroot is mounted - in our space, the paths are still valid, and since the mounts - aren't recursive, it sees *only* the built root, not anything - we would normally se if we rsync'd to "/" like mounted /dev - files. */ - if (do_postclean) - rsync (pristine_root_path, new_root_path, 1); - if (WIFEXITED (status)) exit (WEXITSTATUS (status));