[pacman-dev] [PATCH] makepkg: Delete logpipe when cleaning up

Austin Lund

26 Aug 2019 26 Aug '19

11:29 a.m.

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. Signed-off-by: Austin Lund --- 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" + if (( (EXIT_CODE == E_OK || EXIT_CODE == E_INSTALL_FAILED) && CLEANUP )); then local pkg file @@ -343,9 +345,6 @@ remove_deps() { } error_function() { - if [[ -p $logpipe ]]; then - rm "$logpipe" - fi # first exit all subshells, then print the error if (( ! BASH_SUBSHELL )); then error "$(gettext "A failure occurred in %s().")" "$1" -- 2.23.0

Show replies by date

Allan McRae

26 Aug 26 Aug

11:42 a.m.

On 26/8/19 9:29 pm, 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.

Can you provide an example that is not caught by error_function()? A

Austin Lund

12:03 p.m.

On Mon, 26 Aug 2019 at 21:42, Allan McRae wrote:

...

On 26/8/19 9:29 pm, 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.

Can you provide an example that is not caught by error_function()?

SIGINT and SIGUSR1 leaves the fifo in the log dir for me.

Eli Schwartz

1:56 p.m.

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?

...

Signed-off-by: Austin Lund --- 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.

...

if (( (EXIT_CODE == E_OK || EXIT_CODE == E_INSTALL_FAILED) && CLEANUP )); then local pkg file

@@ -343,9 +345,6 @@ remove_deps() { }

error_function() { - if [[ -p $logpipe ]]; then - rm "$logpipe" - fi # first exit all subshells, then print the error if (( ! BASH_SUBSHELL )); then error "$(gettext "A failure occurred in %s().")" "$1"

-- Eli Schwartz Bug Wrangler and Trusted User

Eli Schwartz

2:24 p.m.

On 8/26/19 9:56 AM, Eli Schwartz wrote:

...

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.

Ah, I'm blind and did not notice the followup emails. :/ On 8/26/19 8:03 AM, Austin Lund wrote:

...

On Mon, 26 Aug 2019 at 21:42, Allan McRae wrote:

...
Can you provide an example that is not caught by error_function()?

SIGINT and SIGUSR1 leaves the fifo in the log dir for me.

Interesting point. No ERR happened. :/ -- Eli Schwartz Bug Wrangler and Trusted User

Austin Lund

10:20 p.m.

On Mon, 26 Aug 2019 at 23:56, Eli Schwartz 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 --- 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.

Eli Schwartz

10:31 p.m.

On 8/26/19 6:20 PM, Austin Lund wrote:

...

I can trigger it with just basic makepkg followed by Ctrl-C to trigger SIGINT or something like makepkg & sleep 5; kill -USR1 $! .

Please do add that to the commit message, though! Since it was plainly unclear to people what you meant by "a number of exit paths", I consider this important information to record in the commit message history.

...

...
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.

Thanks. -- Eli Schwartz Bug Wrangler and Trusted User

1715

Age (days ago)

1715

Last active (days ago)

List overview

Download

6 comments

3 participants

participants (3)

Allan McRae
Austin Lund
Eli Schwartz