| Message ID | 20240202065740.68643-3-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Multi-migration support | expand |
On 02/02/2024 07.57, Nicholas Piggin wrote: > Migration files weren't being removed when tests were interrupted. > This improves the situation. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > scripts/arch-run.bash | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash > index d0864360..f22ead6f 100644 > --- a/scripts/arch-run.bash > +++ b/scripts/arch-run.bash > @@ -134,12 +134,14 @@ run_migration () > qmp1=$(mktemp -u -t mig-helper-qmp1.XXXXXXXXXX) > qmp2=$(mktemp -u -t mig-helper-qmp2.XXXXXXXXXX) > fifo=$(mktemp -u -t mig-helper-fifo.XXXXXXXXXX) > + > + # race here between file creation and trap > + trap "trap - TERM ; kill 0 ; exit 2" INT TERM > + trap "rm -f ${migout1} ${migsock} ${qmp1} ${qmp2} ${fifo}" RETURN EXIT > + > qmpout1=/dev/null > qmpout2=/dev/null > > - trap 'kill 0; exit 2' INT TERM > - trap 'rm -f ${migout1} ${migsock} ${qmp1} ${qmp2} ${fifo}' RETURN EXIT > - > eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \ > -mon chardev=mon1,mode=control | tee ${migout1} & > live_pid=`jobs -l %+ | grep "eval" | awk '{print$2}'` > @@ -211,8 +213,8 @@ run_panic () > > qmp=$(mktemp -u -t panic-qmp.XXXXXXXXXX) > > - trap 'kill 0; exit 2' INT TERM > - trap 'rm -f ${qmp}' RETURN EXIT > + trap "trap - TERM ; kill 0 ; exit 2" INT TERM > + trap "rm -f ${qmp}" RETURN EXIT > > # start VM stopped so we don't miss any events > eval "$@" -chardev socket,id=mon1,path=${qmp},server=on,wait=off \ So the point is that the "EXIT" trap wasn't executed without the "trap - TERM" in the other trap? ... ok, then your patch certainly makes sense. Reviewed-by: Thomas Huth <thuth@redhat.com>
On Wed Feb 7, 2024 at 5:58 PM AEST, Thomas Huth wrote: > On 02/02/2024 07.57, Nicholas Piggin wrote: > > Migration files weren't being removed when tests were interrupted. > > This improves the situation. > > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > > --- > > scripts/arch-run.bash | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash > > index d0864360..f22ead6f 100644 > > --- a/scripts/arch-run.bash > > +++ b/scripts/arch-run.bash > > @@ -134,12 +134,14 @@ run_migration () > > qmp1=$(mktemp -u -t mig-helper-qmp1.XXXXXXXXXX) > > qmp2=$(mktemp -u -t mig-helper-qmp2.XXXXXXXXXX) > > fifo=$(mktemp -u -t mig-helper-fifo.XXXXXXXXXX) > > + > > + # race here between file creation and trap > > + trap "trap - TERM ; kill 0 ; exit 2" INT TERM > > + trap "rm -f ${migout1} ${migsock} ${qmp1} ${qmp2} ${fifo}" RETURN EXIT > > + > > qmpout1=/dev/null > > qmpout2=/dev/null > > > > - trap 'kill 0; exit 2' INT TERM > > - trap 'rm -f ${migout1} ${migsock} ${qmp1} ${qmp2} ${fifo}' RETURN EXIT > > - > > eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \ > > -mon chardev=mon1,mode=control | tee ${migout1} & > > live_pid=`jobs -l %+ | grep "eval" | awk '{print$2}'` > > @@ -211,8 +213,8 @@ run_panic () > > > > qmp=$(mktemp -u -t panic-qmp.XXXXXXXXXX) > > > > - trap 'kill 0; exit 2' INT TERM > > - trap 'rm -f ${qmp}' RETURN EXIT > > + trap "trap - TERM ; kill 0 ; exit 2" INT TERM > > + trap "rm -f ${qmp}" RETURN EXIT > > > > # start VM stopped so we don't miss any events > > eval "$@" -chardev socket,id=mon1,path=${qmp},server=on,wait=off \ > > So the point is that the "EXIT" trap wasn't executed without the "trap - > TERM" in the other trap? ... ok, then your patch certainly makes sense. Iff you don't remove the TERM handler then the kill will recursively invoke it until some crash. This did solve some cases of dangling temp files for me, although now I test with a simple script: #!/bin/bash trap 'echo "INT" ; kill 0 ; exit 2' INT trap 'trap - TERM ; echo "TERM" ; kill 0 ; exit 2' TERM trap 'echo "RETURN"' RETURN trap 'echo "EXIT"' EXIT sleep 10 echo "done" If you ^C it then it still doesn't get to the EXIT or RETURN handlers. It looks like 'kill -INT $$' might be the way to do it instad of kill 0. Not sure if that means my observation was incorrect, or if the real script is behaving differently. In any case, I will dig into it and try to explain more precisely in the changelog what it is fixing. And possibly do another patch for the 'kill -INT $$' if that is needed. Thanks, Nick
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash index d0864360..f22ead6f 100644 --- a/scripts/arch-run.bash +++ b/scripts/arch-run.bash @@ -134,12 +134,14 @@ run_migration () qmp1=$(mktemp -u -t mig-helper-qmp1.XXXXXXXXXX) qmp2=$(mktemp -u -t mig-helper-qmp2.XXXXXXXXXX) fifo=$(mktemp -u -t mig-helper-fifo.XXXXXXXXXX) + + # race here between file creation and trap + trap "trap - TERM ; kill 0 ; exit 2" INT TERM + trap "rm -f ${migout1} ${migsock} ${qmp1} ${qmp2} ${fifo}" RETURN EXIT + qmpout1=/dev/null qmpout2=/dev/null - trap 'kill 0; exit 2' INT TERM - trap 'rm -f ${migout1} ${migsock} ${qmp1} ${qmp2} ${fifo}' RETURN EXIT - eval "$@" -chardev socket,id=mon1,path=${qmp1},server=on,wait=off \ -mon chardev=mon1,mode=control | tee ${migout1} & live_pid=`jobs -l %+ | grep "eval" | awk '{print$2}'` @@ -211,8 +213,8 @@ run_panic () qmp=$(mktemp -u -t panic-qmp.XXXXXXXXXX) - trap 'kill 0; exit 2' INT TERM - trap 'rm -f ${qmp}' RETURN EXIT + trap "trap - TERM ; kill 0 ; exit 2" INT TERM + trap "rm -f ${qmp}" RETURN EXIT # start VM stopped so we don't miss any events eval "$@" -chardev socket,id=mon1,path=${qmp},server=on,wait=off \
Migration files weren't being removed when tests were interrupted. This improves the situation. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- scripts/arch-run.bash | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)