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

Message ID 20190826112905.26503-1-austin.lund@gmail.com
State Superseded, archived
Headers show
Series
  • [pacman-dev] makepkg: Delete logpipe when cleaning up
Related show

Commit Message

Austin Lund Aug. 26, 2019, 11:29 a.m. UTC
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 <austin.lund@gmail.com>
---
 scripts/makepkg.sh.in | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Allan McRae Aug. 26, 2019, 11:42 a.m. UTC | #1
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 Aug. 26, 2019, 12:03 p.m. UTC | #2
On Mon, 26 Aug 2019 at 21:42, Allan McRae <allan@archlinux.org> 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 Aug. 26, 2019, 1:56 p.m. UTC | #3
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 <austin.lund@gmail.com>
> ---
>  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 Aug. 26, 2019, 2:24 p.m. UTC | #4
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 <allan@archlinux.org> 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. :/
Austin Lund Aug. 26, 2019, 10:20 p.m. UTC | #5
On Mon, 26 Aug 2019 at 23:56, Eli Schwartz <eschwartz@archlinux.org> 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 <austin.lund@gmail.com>
> > ---
> >  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 Aug. 26, 2019, 10:31 p.m. UTC | #6
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.

Patch

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"