[PATCH 2/2] test/py: Wait for guestmount worker to exit after running guestunmount

[Simon Glass]

On Fri, 4 Jun 2021 at 13:05, Alper Nebi Yasak <alpernebiyasak at gmail.com> wrote:
>
> Some filesystem tests are failing when their image is prepared with
> guestmount, but succeeding if loop mounts are used instead. The reason
> seems to be a race condition the guestmount(1) manual page explains:
>
>     When guestunmount(1)/fusermount(1) exits, guestmount may still be
>     running and cleaning up the mountpoint.  The disk image will not be
>     fully finalized.
>
>     This means that scripts like the following have a nasty race condition:
>
>      guestmount -a disk.img -i /mnt
>      # copy things into /mnt
>      guestunmount /mnt
>      # immediately try to use 'disk.img' ** UNSAFE **
>
>     The solution is to use the --pid-file option to write the guestmount
>     PID to a file, then after guestunmount spin waiting for this PID to
>     exit.
>
> The Python standard library has an os.waitpid() function for waiting a
> child to terminate, but it cannot wait on non-child processes. Implement
> a utility function that can do this by polling the process repeatedly
> for a given duration, optionally killing the process if it won't
> terminate on its own. Apply the suggested solution with this utility
> function, which makes the failing tests succeed again.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak at gmail.com>
> ---
>
>  test/py/tests/test_fs/conftest.py | 13 ++++++++++-
>  test/py/u_boot_utils.py           | 36 +++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 1 deletion(-)
>

Reviewed-by: Simon Glass <sjg at chromium.org>

> diff --git a/test/py/tests/test_fs/conftest.py b/test/py/tests/test_fs/conftest.py
> index e3c461635f8e..6b1ff05a8143 100644
> --- a/test/py/tests/test_fs/conftest.py
> +++ b/test/py/tests/test_fs/conftest.py
> @@ -8,6 +8,7 @@
>  import re
>  from subprocess import call, check_call, check_output, CalledProcessError
>  from fstest_defs import *
> +import u_boot_utils as util
>
>  supported_fs_basic = ['fat16', 'fat32', 'ext4']
>  supported_fs_ext = ['fat16', 'fat32']
> @@ -206,7 +207,7 @@ def mount_fs(fs_type, device, mount_point):
>      global fuse_mounted
>
>      try:
> -        check_call('guestmount -a %s -m /dev/sda %s'
> +        check_call('guestmount --pid-file guestmount.pid -a %s -m /dev/sda %s'
>              % (device, mount_point), shell=True)
>          fuse_mounted = True
>          return
> @@ -235,6 +236,16 @@ def umount_fs(mount_point):
>      if fuse_mounted:
>          call('sync')

should you remove guestmount.pid first in case it is there from a
previous crash?


>          call('guestunmount %s' % mount_point, shell=True)
> +
> +        try:
> +            with open("guestmount.pid", "r") as pidfile:
> +                pid = int(pidfile.read())
> +            util.waitpid(pid, kill=True)
> +            os.remove("guestmount.pid")
> +
> +        except FileNotFoundError:
> +            pass
> +
>      else:
>          call('sudo umount %s' % mount_point, shell=True)
>
> diff --git a/test/py/u_boot_utils.py b/test/py/u_boot_utils.py
> index 939d82eec12a..e816c7fbb6a3 100644
> --- a/test/py/u_boot_utils.py
> +++ b/test/py/u_boot_utils.py
> @@ -8,6 +8,7 @@
>  import os
>  import os.path
>  import pytest
> +import signal
>  import sys
>  import time
>  import re
> @@ -339,3 +340,38 @@ def crc32(u_boot_console, address, count):
>      assert m, 'CRC32 operation failed.'
>
>      return m.group(1)
> +
> +def waitpid(pid, timeout=60, kill=False):
> +    """Wait a process to terminate by its PID
> +
> +    This is an alternative to a os.waitpid(pid, 0) call that works on
> +    processes that aren't children of the python process.
> +
> +    Args:
> +        pid: PID of a running process.
> +        timeout: Time in seconds to wait.
> +        kill: Whether to forcibly kill the process after timeout.
> +
> +    Returns:
> +        True, if the process ended on its own.
> +        False, if the process was killed by this function.
> +
> +    Raises:
> +        TimeoutError, if the process is still running after timeout.
> +    """
> +    try:
> +        for _ in range(timeout):
> +            os.kill(pid, 0)
> +            time.sleep(1)
> +
> +        if kill:
> +            os.kill(pid, signal.SIGKILL)
> +            return False
> +
> +    except ProcessLookupError:
> +        return True
> +
> +    raise TimeoutError(
> +        "Process with PID {} did not terminate after {} seconds."
> +        .format(pid, timeout)
> +    )
> --
> 2.32.0.rc2
>