On Mon, 26 Aug 2019 at 23:56, Eli Schwartz <[email protected]> wrote: > > On 8/26/19 7:29 AM, Austin Lund wrote: > > There are a number of exit paths where the logpipe fifo remains in the > > logging directory. Remove it if it exists when cleaning up at exit. > > Sounds like https://patchwork.archlinux.org/patch/1171/ again. > > error_function() should always run on the errexit trap for > run_function_safe, and inside run_function_safe, run_function will run > to execute the function, log the logpipe, and rm the logpipe if > everything succeeded. If for any reason a call in run_function fails > (even the `rm "$logpipe"`) the error_function trap should definitely > run, and delete the logpipe fifo. > > The only way it should be left behind is if e.g. this exits without > running the final rm, but does so without errexit erroring -- how???: > > mkfifo "$logpipe" > tee "$BUILDLOG" < "$logpipe" & > local teepid=$! > > $pkgfunc &>"$logpipe" > > wait $teepid > rm "$logpipe" > > > Alternatively, if the rm itself errors -- that is what the other patch > is about, but why would it error? > > Alternatively, makepkg is killed in such a way that it isn't allowed to > try running cleanup traps. If the process is SIGKILLed, then > error_function will not run, but neither will clean_up. > > I would really like to know in what situations this is supposed to fail. > e.g. Is it only in devtools?
I can trigger it with just basic makepkg followed by Ctrl-C to trigger SIGINT or something like makepkg & sleep 5; kill -USR1 $! . > > > Signed-off-by: Austin Lund <[email protected]> > > --- > > scripts/makepkg.sh.in | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in > > index 43484db3..0e0d3461 100644 > > --- a/scripts/makepkg.sh.in > > +++ b/scripts/makepkg.sh.in > > @@ -129,6 +129,8 @@ clean_up() { > > return 0 > > fi > > > > + [[ -p $logpipe ]] && rm "$logpipe" > > + > > Why switch from > if [[ -p $logpipe ]]; then rm "$logpipe"; fi > to > [[ -p $logpipe ]] && rm "$logpipe" > > Seems a bit silly to add new cases of `return 1` to our cleanup function. Indeed. I forgot the special return behaviour of 'if' constructs when the conditions are false. I will submit a revised patch.
