All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] tests: Let grub_cmd_cryptomount by default operate in /tmp rather than in /
@ 2024-07-24 16:48 Thomas Schmitt via Grub-devel
  2024-08-13 13:59 ` Daniel Kiper
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Schmitt via Grub-devel @ 2024-07-24 16:48 UTC (permalink / raw)
  To: grub-devel; +Cc: Thomas Schmitt, development

If not TMPDIR is set by the user then the test grub_cmd_cryptomount
creates about 20 directories named *LUKS*_test* in the root directory
and leaves them there when the test ends.

Initialize in the test script the variable TMPDIR to /tmp if it is not
set or if it set to empty text. To be consistent with the usage of
${TMPDIR:-/tmp} in the script, use ${TMPDIR:=/tmp} not ${TMPDIR=/tmp}.

Further delete each created directory as soon as the command of its
test case is finished.

Signed-off-by: Thomas Schmitt <scdbackup@gmx.net>
---
 tests/grub_cmd_cryptomount.in | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tests/grub_cmd_cryptomount.in b/tests/grub_cmd_cryptomount.in
index f4d8f3547..696e61e96 100644
--- a/tests/grub_cmd_cryptomount.in
+++ b/tests/grub_cmd_cryptomount.in
@@ -44,12 +44,23 @@ _testcase() {
     local output
     shift 2

+    # Use the environment variable TMPDIR, falling back to /tmp. This allows
+    # users to specify a different temporary directory, for example, if their
+    # /tmp is filled up or too small.
+    # Some other GRUB tests use this gesture with "=" rather than ":=".
+    # But in sync with the many occurences of ${TMPDIR:-/tmp}, this test uses
+    # ":=" to fill empty TMPDIR with "/tmp", regardless whether TMPDIR was
+    # set to empty or was not set at all.
+    : ${TMPDIR:=/tmp}
+
     # Create a subdir in TMPDIR for each testcase
     _TMPDIR=$TMPDIR
     TMPDIR=$TMPDIR/`echo -n "$(date +%s).$LOGPREFIX" | sed -e 's,[ /],_,g' -e 's,:$,,g'`
     mkdir -p "$TMPDIR"

     output=`"$@" 2>&1` || res=$?
+
+    rmdir "$TMPDIR"
     TMPDIR=$_TMPDIR

     if [ "$res" -eq "$EXPECTEDRES" ]; then
--
2.39.2


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] tests: Let grub_cmd_cryptomount by default operate in /tmp rather than in /
  2024-07-24 16:48 [PATCH] tests: Let grub_cmd_cryptomount by default operate in /tmp rather than in / Thomas Schmitt via Grub-devel
@ 2024-08-13 13:59 ` Daniel Kiper
  2024-08-13 15:11   ` Thomas Schmitt via Grub-devel
  2024-08-13 16:14   ` Thomas Schmitt via Grub-devel
  0 siblings, 2 replies; 9+ messages in thread
From: Daniel Kiper @ 2024-08-13 13:59 UTC (permalink / raw)
  To: Thomas Schmitt, development; +Cc: grub-devel

On Wed, Jul 24, 2024 at 06:48:45PM +0200, Thomas Schmitt via Grub-devel wrote:
> If not TMPDIR is set by the user then the test grub_cmd_cryptomount
> creates about 20 directories named *LUKS*_test* in the root directory
> and leaves them there when the test ends.
>
> Initialize in the test script the variable TMPDIR to /tmp if it is not
> set or if it set to empty text. To be consistent with the usage of
> ${TMPDIR:-/tmp} in the script, use ${TMPDIR:=/tmp} not ${TMPDIR=/tmp}.
>
> Further delete each created directory as soon as the command of its
> test case is finished.
>
> Signed-off-by: Thomas Schmitt <scdbackup@gmx.net>
> ---
>  tests/grub_cmd_cryptomount.in | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/tests/grub_cmd_cryptomount.in b/tests/grub_cmd_cryptomount.in
> index f4d8f3547..696e61e96 100644
> --- a/tests/grub_cmd_cryptomount.in
> +++ b/tests/grub_cmd_cryptomount.in
> @@ -44,12 +44,23 @@ _testcase() {
>      local output
>      shift 2
>
> +    # Use the environment variable TMPDIR, falling back to /tmp. This allows
> +    # users to specify a different temporary directory, for example, if their
> +    # /tmp is filled up or too small.
> +    # Some other GRUB tests use this gesture with "=" rather than ":=".
> +    # But in sync with the many occurences of ${TMPDIR:-/tmp}, this test uses
> +    # ":=" to fill empty TMPDIR with "/tmp", regardless whether TMPDIR was
> +    # set to empty or was not set at all.
> +    : ${TMPDIR:=/tmp}
> +
>      # Create a subdir in TMPDIR for each testcase
>      _TMPDIR=$TMPDIR
>      TMPDIR=$TMPDIR/`echo -n "$(date +%s).$LOGPREFIX" | sed -e 's,[ /],_,g' -e 's,:$,,g'`
>      mkdir -p "$TMPDIR"
>
>      output=`"$@" 2>&1` || res=$?
> +
> +    rmdir "$TMPDIR"

s/rmdir/rm -rf/?

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] tests: Let grub_cmd_cryptomount by default operate in /tmp rather than in /
  2024-08-13 13:59 ` Daniel Kiper
@ 2024-08-13 15:11   ` Thomas Schmitt via Grub-devel
  2024-08-14 11:50     ` Daniel Kiper
  2024-08-13 16:14   ` Thomas Schmitt via Grub-devel
  1 sibling, 1 reply; 9+ messages in thread
From: Thomas Schmitt via Grub-devel @ 2024-08-13 15:11 UTC (permalink / raw)
  To: grub-devel; +Cc: Thomas Schmitt, development, dkiper

Hi,

i wrote:
> > Further delete each created directory as soon as the command of its
> > test case is finished.
> > [...]
> >      mkdir -p "$TMPDIR"
> >
> >      output=`"$@" 2>&1` || res=$?
> > +
> > +    rmdir "$TMPDIR"

Daniel Kiper wrote:
> s/rmdir/rm -rf/?

This is equivalent to the question whether remaining content shall be
removed silently. In my case the directories were all empty.

The worker in TMPDIR is  @builddir@/grub-shell-luks-tester  which stems
from  tests/util/grub-shell-luks-tester.in . It shows own effort to leave
a clean TMPDIR:

  cleanup() {
      ...
      if [ -z "$debug" ] && [ "${RET:-1}" -eq 0 ]; then
          rm -rf "$lukstestdir" || :
      fi
  }
  trap cleanup EXIT INT TERM KILL QUIT
  ...
  lukstestdir="`mktemp -d "${TMPDIR:-/tmp}/$(basename "$0").XXXXXXXXXX"`" || exit 20

Seeing the condition before "rm -rf", i guess tests/grub_cmd_cryptomount
should rather not remove the directory if it is not empty. A smarter way
than letting rmdir loudly fail seems appropriate, though.

If you agree to my assessment i will try to propose one in v2.

-----------------------------------------------------------------------
Self criticism:

I have recognized meanwhile that the proposed gesture

> > +    : ${TMPDIR:=/tmp}

stems from the gnulib subdirectory and is not tradition in the rest
of the GRUB git repo.
GRUB test tradition seems to be temporary defaulting of TMPDIR like in
line 174 of tests/grub_cmd_cryptomount.in :

  csscript=`mktemp "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` || exit 99

Indeed the patch would be less needy of a lengthy comment if i propose
something like this yet untested change instead:

-    TMPDIR=$TMPDIR/`echo -n "$(date +%s).$LOGPREFIX" | sed -e 's,[ /],_,g' -e 's,:$,,g'`
+    TMPDIR="${TMPDIR:-/tmp}"/`echo -n "$(date +%s).$LOGPREFIX" | sed -e 's,[ /],_,g' -e 's,:$,,g'`

If you agree to my assessment, i will propose this in patch v2.
(Tested, of course.)


Have a nice day :)

Thomas


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] tests: Let grub_cmd_cryptomount by default operate in /tmp rather than in /
  2024-08-13 13:59 ` Daniel Kiper
  2024-08-13 15:11   ` Thomas Schmitt via Grub-devel
@ 2024-08-13 16:14   ` Thomas Schmitt via Grub-devel
  2024-08-14 11:54     ` Daniel Kiper
  1 sibling, 1 reply; 9+ messages in thread
From: Thomas Schmitt via Grub-devel @ 2024-08-13 16:14 UTC (permalink / raw)
  To: grub-devel; +Cc: Thomas Schmitt, development, dkiper

Hi,

thinking more i believe that the currently used mkdir option -p is
inappropriate in  tests/grub_cmd_cryptomount.in .

It hampers proper cleanup because the script cannot know how many
directories in the path to TMPDIR were created and need to be removed.
It is unusual because about all other tests in GRUB assume that non-empty
TMPDIR or its default "/tmp" lead to an existing directory.
(At least my local mktemp(1) refuses on non-existing components in
the path.)

If i read no objections, i will propose removal of -p in patch v2.


Have a nice day :)

Thomas


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] tests: Let grub_cmd_cryptomount by default operate in /tmp rather than in /
  2024-08-13 15:11   ` Thomas Schmitt via Grub-devel
@ 2024-08-14 11:50     ` Daniel Kiper
  0 siblings, 0 replies; 9+ messages in thread
From: Daniel Kiper @ 2024-08-14 11:50 UTC (permalink / raw)
  To: Thomas Schmitt; +Cc: grub-devel, development

On Tue, Aug 13, 2024 at 05:11:34PM +0200, Thomas Schmitt wrote:
> Hi,
>
> i wrote:
> > > Further delete each created directory as soon as the command of its
> > > test case is finished.
> > > [...]
> > >      mkdir -p "$TMPDIR"
> > >
> > >      output=`"$@" 2>&1` || res=$?
> > > +
> > > +    rmdir "$TMPDIR"
>
> Daniel Kiper wrote:
> > s/rmdir/rm -rf/?
>
> This is equivalent to the question whether remaining content shall be
> removed silently. In my case the directories were all empty.
>
> The worker in TMPDIR is  @builddir@/grub-shell-luks-tester  which stems
> from  tests/util/grub-shell-luks-tester.in . It shows own effort to leave
> a clean TMPDIR:
>
>   cleanup() {
>       ...
>       if [ -z "$debug" ] && [ "${RET:-1}" -eq 0 ]; then
>           rm -rf "$lukstestdir" || :
>       fi
>   }
>   trap cleanup EXIT INT TERM KILL QUIT
>   ...
>   lukstestdir="`mktemp -d "${TMPDIR:-/tmp}/$(basename "$0").XXXXXXXXXX"`" || exit 20
>
> Seeing the condition before "rm -rf", i guess tests/grub_cmd_cryptomount
> should rather not remove the directory if it is not empty. A smarter way
> than letting rmdir loudly fail seems appropriate, though.
>
> If you agree to my assessment i will try to propose one in v2.

OK...

> -----------------------------------------------------------------------
> Self criticism:
>
> I have recognized meanwhile that the proposed gesture
>
> > > +    : ${TMPDIR:=/tmp}
>
> stems from the gnulib subdirectory and is not tradition in the rest
> of the GRUB git repo.
> GRUB test tradition seems to be temporary defaulting of TMPDIR like in
> line 174 of tests/grub_cmd_cryptomount.in :
>
>   csscript=`mktemp "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` || exit 99
>
> Indeed the patch would be less needy of a lengthy comment if i propose
> something like this yet untested change instead:
>
> -    TMPDIR=$TMPDIR/`echo -n "$(date +%s).$LOGPREFIX" | sed -e 's,[ /],_,g' -e 's,:$,,g'`
> +    TMPDIR="${TMPDIR:-/tmp}"/`echo -n "$(date +%s).$LOGPREFIX" | sed -e 's,[ /],_,g' -e 's,:$,,g'`
>
> If you agree to my assessment, i will propose this in patch v2.
> (Tested, of course.)

OK...

Daniel

PS I have fixed Glenn's email address...

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] tests: Let grub_cmd_cryptomount by default operate in /tmp rather than in /
  2024-08-13 16:14   ` Thomas Schmitt via Grub-devel
@ 2024-08-14 11:54     ` Daniel Kiper
  2024-08-18 20:09       ` Thomas Schmitt via Grub-devel
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Kiper @ 2024-08-14 11:54 UTC (permalink / raw)
  To: Thomas Schmitt; +Cc: grub-devel, development, phcoder

On Tue, Aug 13, 2024 at 06:14:40PM +0200, Thomas Schmitt wrote:
> Hi,
>
> thinking more i believe that the currently used mkdir option -p is
> inappropriate in  tests/grub_cmd_cryptomount.in .
>
> It hampers proper cleanup because the script cannot know how many
> directories in the path to TMPDIR were created and need to be removed.
> It is unusual because about all other tests in GRUB assume that non-empty
> TMPDIR or its default "/tmp" lead to an existing directory.
> (At least my local mktemp(1) refuses on non-existing components in
> the path.)
>
> If i read no objections, i will propose removal of -p in patch v2.

I am OK with this change but I would want to hear Glenn's and Vladimir's
opinion here.

Daniel

PS I have fixed Glenn's email address...

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] tests: Let grub_cmd_cryptomount by default operate in /tmp rather than in /
  2024-08-14 11:54     ` Daniel Kiper
@ 2024-08-18 20:09       ` Thomas Schmitt via Grub-devel
  2024-08-18 21:41         ` Thomas Schmitt via Grub-devel
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Schmitt via Grub-devel @ 2024-08-18 20:09 UTC (permalink / raw)
  To: dkiper; +Cc: Thomas Schmitt, phcoder, development, grub-devel

Hi,

the submission of v2 is hampered by the fact that the artful composition
of TMPDIR in tests/grub_cmd_cryptomount.in does not influence the worker
tests/util/grub-shell-luks-tester.in because TMPDIR is not exported.

So grub-shell-luks-tester creates its workdirectory directly in /tmp
rather than in /tmp/1724008906.LUKS1_test_key_file_with_offset_and_size/ .

More problems show up:
-------------------------------------------------------------------------

grub-shell-luks-tester does _not_ clean up.
That's because in a run of
  make check TESTS=grub_cmd_cryptomount
the situation in function cleanup() of grub-shell-luks-tester is:

  lukstestdir='/tmp/grub-shell-luks-tester.S2rhmuJJlf'
  debug=''
  RET=''
  {RET:-1}='1'

So the condition in the following if-clause is false because [ 1 -eq 0 ]:

  if [ -z "$debug" ] && [ "${RET:-1}" -eq 0 ]; then
      rm -rf "$lukstestdir" || :
  fi

and no rm -rf happens.

I riddle from where RET shall get its expected content 0.

-------------------------------------------------------------------------

After each make check round, another single directory is left:

  /tmp/tmp.yp1z11pq2w

of the same age as the grub-shell-luks-tester directories.
It stems from this gesture in grub-shell-luks-tester.in:

  # Add good password to second slot and change first slot to unchecked password
  csscript=`mktemp "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` || exit 99

I proved this by changing "tmp.XXXXXXXXXX" to "tmp_css.XXXXXXXXXX" and
then getting  /tmp/tmp_css.5wRYh4CUaL .

-------------------------------------------------------------------------

Another problem is that the test leaves empty directories of the form

  /tmp/grub-shell.2jWNHEhpA2

(which also end up in TMPDIR if it is exported).

-------------------------------------------------------------------------

So i will have to investigate more ... and would not mind if others
would take this issue out of my hands.


Have a nice day :)

Thomas


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] tests: Let grub_cmd_cryptomount by default operate in /tmp rather than in /
  2024-08-18 20:09       ` Thomas Schmitt via Grub-devel
@ 2024-08-18 21:41         ` Thomas Schmitt via Grub-devel
  2024-09-23  5:57           ` Glenn Washburn
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Schmitt via Grub-devel @ 2024-08-18 21:41 UTC (permalink / raw)
  To: dkiper; +Cc: Thomas Schmitt, phcoder, development, grub-devel

Hi,

i post this as base of discussion, not yet as sincere patch proposal.

It seems desirable to bundle the temporary data of each particular test
in tests/grub_cmd_cryptomount.in in a single directory, as it seems
intended by the code but currently is not achieved.
So i propose to default TMPDIR to "/tmp" and to export it after it
was augmented by a test specific subdirectory component.

In order to let tests/grub_cmd_cryptomount.in finally remove this work
directory, we can either do "rm -rf" or let the subordinate workers clean
up their work directories which they create as subdirectories.
I am in favor of the latter.

This implies a possibly intrusive change in
  tests/util/grub-shell-luks-tester.in
where i propose to drop the evaluation of a variable named RET, which i
cannot find to be set or used anywhere in the git repo tree.
Another change would be in
  tests/util/grub-shell.in
where i propose to finally remove the work directory.
In order to avoid large colateral damage, i propose rmdir rather than
"rm -rf". (If debris remains in the work directory than this should be
investigated by the user.)

Finally i propose to remove a regular file which the last test of
grub_cmd_cryptomount.in currently leaves in /tmp.

I add a diff to the code which now works for me and leaves no debris
in /tmp after
  make check TESTS=grub_cmd_cryptomount
In case somebody finds parts of it useful:
  Signed-off-by: Thomas Schmitt <scdbackup@gmx.net>


Have a nice day :)

Thomas

------------------------------------------------------------------------

diff --git a/tests/grub_cmd_cryptomount.in b/tests/grub_cmd_cryptomount.in
index f4d8f3547..5ce97fa06 100644
--- a/tests/grub_cmd_cryptomount.in
+++ b/tests/grub_cmd_cryptomount.in
@@ -37,6 +37,8 @@ fi

 COMMON_OPTS='${V:+--debug=$V} --cs-opts="--pbkdf-force-iterations 1000"'

+debug=${GRUB_SHELL_DEFAULT_DEBUG:-$GRUB_TEST_DEFAULT_DEBUG}
+
 _testcase() {
     local EXPECTEDRES=$1
     local LOGPREFIX=$2
@@ -46,10 +48,22 @@ _testcase() {

     # Create a subdir in TMPDIR for each testcase
     _TMPDIR=$TMPDIR
-    TMPDIR=$TMPDIR/`echo -n "$(date +%s).$LOGPREFIX" | sed -e 's,[ /],_,g' -e 's,:$,,g'`
+    TMPDIR="${TMPDIR:-/tmp}"/`echo -n "$(date +%s).$LOGPREFIX" | sed -e 's,[ /],_,g' -e 's,:$,,g'`
+    export TMPDIR
     mkdir -p "$TMPDIR"

     output=`"$@" 2>&1` || res=$?
+
+    if [ -z "$debug" ]; then
+        if ! rmdir "$TMPDIR" >/dev/null 2>&1; then
+            echo
+            echo "Note: Temporary directory cannot be removed:"
+            echo "        $TMPDIR"
+            echo "      Please inspect and remove manually."
+            echo
+        fi
+    fi
+    unset TMPDIR
     TMPDIR=$_TMPDIR

     if [ "$res" -eq "$EXPECTEDRES" ]; then
@@ -182,4 +196,6 @@ eval testcase "'LUKS2 test with second key slot and first slot using different p
     @builddir@/grub-shell-luks-tester $LUKS2_COMMON_OPTS $COMMON_OPTS \
         "--cs-script='$csscript'"

+test -n "$debug" || rm "$csscript"
+
 exit 0
diff --git a/tests/util/grub-shell-luks-tester.in b/tests/util/grub-shell-luks-tester.in
index b2a8a91b4..20ebecacf 100644
--- a/tests/util/grub-shell-luks-tester.in
+++ b/tests/util/grub-shell-luks-tester.in
@@ -146,7 +146,7 @@ cleanup() {
     if [ -e "$luksdev" ]; then
         cryptsetup close "$luksdev"
     fi
-    if [ -z "$debug" ] && [ "${RET:-1}" -eq 0 ]; then
+    if [ -z "$debug" ] ; then
         rm -rf "$lukstestdir" || :
     fi
 }
diff --git a/tests/util/grub-shell.in b/tests/util/grub-shell.in
index ae5f711fe..98f7e7394 100644
--- a/tests/util/grub-shell.in
+++ b/tests/util/grub-shell.in
@@ -711,6 +711,7 @@ test -n "$debug" || rm -f "${isofile}"
 test -n "$debug" || rm -rf "${rom_directory}"
 test -n "$debug" || rm -f "${tmpfile}" "${cfgfile}" "${goutfile}"
 test -n "$debug" || rm -f "$work_directory/run.sh"
+test -n "$debug" || rmdir "$work_directory"

 exit $ret


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] tests: Let grub_cmd_cryptomount by default operate in /tmp rather than in /
  2024-08-18 21:41         ` Thomas Schmitt via Grub-devel
@ 2024-09-23  5:57           ` Glenn Washburn
  0 siblings, 0 replies; 9+ messages in thread
From: Glenn Washburn @ 2024-09-23  5:57 UTC (permalink / raw)
  To: Thomas Schmitt; +Cc: dkiper, phcoder, grub-devel

On Sun, 18 Aug 2024 23:41:16 +0200
"Thomas Schmitt" <scdbackup@gmx.net> wrote:

> Hi,
> 
> i post this as base of discussion, not yet as sincere patch proposal.
> 
> It seems desirable to bundle the temporary data of each particular test
> in tests/grub_cmd_cryptomount.in in a single directory, as it seems
> intended by the code but currently is not achieved.
> So i propose to default TMPDIR to "/tmp" and to export it after it
> was augmented by a test specific subdirectory component.

I think a lot of your headaches comes from not exporting TMPDIR in the
shell that runs the make check. This is an undocumented assumption of
the code. Defaulting to TMPDIR to /tmp as you're suggesting seems like
a good idea.

> 
> In order to let tests/grub_cmd_cryptomount.in finally remove this work
> directory, we can either do "rm -rf" or let the subordinate workers clean
> up their work directories which they create as subdirectories.
> I am in favor of the latter.
> 
> This implies a possibly intrusive change in
>   tests/util/grub-shell-luks-tester.in
> where i propose to drop the evaluation of a variable named RET, which i
> cannot find to be set or used anywhere in the git repo tree.

You caught an oversight on my part. The first line of the cleanup()
function should be 'RET="$?"'. Since cleanup is only called on error or
exit, $? will tell whether the script is exiting in error or not. If it
exits in error, we don't want to cleanup the test so that it can be
further debugged.

> Another change would be in
>   tests/util/grub-shell.in
> where i propose to finally remove the work directory.
> In order to avoid large colateral damage, i propose rmdir rather than
> "rm -rf". (If debris remains in the work directory than this should be
> investigated by the user.)

I'm fine with this.

> 
> Finally i propose to remove a regular file which the last test of
> grub_cmd_cryptomount.in currently leaves in /tmp.

Sounds good.

> 
> I add a diff to the code which now works for me and leaves no debris

I've reworked this into several patches with some improvements. I think
it should work for you, but let me know if you have issues or comments.

Glenn

> in /tmp after
>   make check TESTS=grub_cmd_cryptomount
> In case somebody finds parts of it useful:
>   Signed-off-by: Thomas Schmitt <scdbackup@gmx.net>
> 
> 
> Have a nice day :)
> 
> Thomas
> 
> ------------------------------------------------------------------------
> 
> diff --git a/tests/grub_cmd_cryptomount.in b/tests/grub_cmd_cryptomount.in
> index f4d8f3547..5ce97fa06 100644
> --- a/tests/grub_cmd_cryptomount.in
> +++ b/tests/grub_cmd_cryptomount.in
> @@ -37,6 +37,8 @@ fi
> 
>  COMMON_OPTS='${V:+--debug=$V} --cs-opts="--pbkdf-force-iterations 1000"'
> 
> +debug=${GRUB_SHELL_DEFAULT_DEBUG:-$GRUB_TEST_DEFAULT_DEBUG}
> +
>  _testcase() {
>      local EXPECTEDRES=$1
>      local LOGPREFIX=$2
> @@ -46,10 +48,22 @@ _testcase() {
> 
>      # Create a subdir in TMPDIR for each testcase
>      _TMPDIR=$TMPDIR
> -    TMPDIR=$TMPDIR/`echo -n "$(date +%s).$LOGPREFIX" | sed -e 's,[ /],_,g' -e 's,:$,,g'`
> +    TMPDIR="${TMPDIR:-/tmp}"/`echo -n "$(date +%s).$LOGPREFIX" | sed -e 's,[ /],_,g' -e 's,:$,,g'`
> +    export TMPDIR
>      mkdir -p "$TMPDIR"
> 
>      output=`"$@" 2>&1` || res=$?
> +
> +    if [ -z "$debug" ]; then
> +        if ! rmdir "$TMPDIR" >/dev/null 2>&1; then
> +            echo
> +            echo "Note: Temporary directory cannot be removed:"
> +            echo "        $TMPDIR"
> +            echo "      Please inspect and remove manually."
> +            echo
> +        fi
> +    fi
> +    unset TMPDIR
>      TMPDIR=$_TMPDIR
> 
>      if [ "$res" -eq "$EXPECTEDRES" ]; then
> @@ -182,4 +196,6 @@ eval testcase "'LUKS2 test with second key slot and first slot using different p
>      @builddir@/grub-shell-luks-tester $LUKS2_COMMON_OPTS $COMMON_OPTS \
>          "--cs-script='$csscript'"
> 
> +test -n "$debug" || rm "$csscript"
> +

Looks good.

>  exit 0
> diff --git a/tests/util/grub-shell-luks-tester.in b/tests/util/grub-shell-luks-tester.in
> index b2a8a91b4..20ebecacf 100644
> --- a/tests/util/grub-shell-luks-tester.in
> +++ b/tests/util/grub-shell-luks-tester.in
> @@ -146,7 +146,7 @@ cleanup() {
>      if [ -e "$luksdev" ]; then
>          cryptsetup close "$luksdev"
>      fi
> -    if [ -z "$debug" ] && [ "${RET:-1}" -eq 0 ]; then
> +    if [ -z "$debug" ] ; then
>          rm -rf "$lukstestdir" || :
>      fi
>  }
> diff --git a/tests/util/grub-shell.in b/tests/util/grub-shell.in
> index ae5f711fe..98f7e7394 100644
> --- a/tests/util/grub-shell.in
> +++ b/tests/util/grub-shell.in
> @@ -711,6 +711,7 @@ test -n "$debug" || rm -f "${isofile}"
>  test -n "$debug" || rm -rf "${rom_directory}"
>  test -n "$debug" || rm -f "${tmpfile}" "${cfgfile}" "${goutfile}"
>  test -n "$debug" || rm -f "$work_directory/run.sh"
> +test -n "$debug" || rmdir "$work_directory"
> 
>  exit $ret
> 

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2024-09-23  5:57 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-24 16:48 [PATCH] tests: Let grub_cmd_cryptomount by default operate in /tmp rather than in / Thomas Schmitt via Grub-devel
2024-08-13 13:59 ` Daniel Kiper
2024-08-13 15:11   ` Thomas Schmitt via Grub-devel
2024-08-14 11:50     ` Daniel Kiper
2024-08-13 16:14   ` Thomas Schmitt via Grub-devel
2024-08-14 11:54     ` Daniel Kiper
2024-08-18 20:09       ` Thomas Schmitt via Grub-devel
2024-08-18 21:41         ` Thomas Schmitt via Grub-devel
2024-09-23  5:57           ` Glenn Washburn

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.