[PATCH v2 0/8] testing/next (docker, binfmt, gdb version)

[PATCH v2 0/8] testing/next (docker, binfmt, gdb version)

[Alex Bennée]

Hi,

The testing updates keep on rolling. This expands on the last version
by adding some more handling of weird gdb version numbers and bumping
the minimum requirement again.

The following need review:

 - configure: bump the minimum gdb version for check-tcg to 9.1
 - configure: make version_ge more tolerant of shady version input
 - tests/docker: add a docker-exec-copy-test
 - tests/docker: alias docker-help target for consistency
 - tests/docker: preserve original name when copying libs

Alex Bennée (6):
  tests/docker: make _copy_with_mkdir accept missing files
  tests/docker: preserve original name when copying libs
  tests/docker: alias docker-help target for consistency
  tests/docker: add a docker-exec-copy-test
  configure: make version_ge more tolerant of shady version input
  configure: bump the minimum gdb version for check-tcg to 9.1

Philippe Mathieu-Daudé (2):
  tests/docker: Fix _get_so_libs() for docker-binfmt-image
  tests/docker: Fix typo in help message

 configure                             |  6 +++---
 tests/docker/Makefile.include         | 26 +++++++++++++++++++++++---
 tests/docker/docker.py                | 23 +++++++++++++++++------
 tests/docker/dockerfiles/empty.docker |  8 ++++++++
 4 files changed, 51 insertions(+), 12 deletions(-)
 create mode 100644 tests/docker/dockerfiles/empty.docker

-- 
2.20.1

[PATCH v2 1/8] tests/docker: Fix _get_so_libs() for docker-binfmt-image

[Alex Bennée]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

Fix a variable rename mistake from commit 5e33f7fead5:

  Traceback (most recent call last):
    File "./tests/docker/docker.py", line 710, in <module>
      sys.exit(main())
    File "./tests/docker/docker.py", line 706, in main
      return args.cmdobj.run(args, argv)
    File "./tests/docker/docker.py", line 489, in run
      _copy_binary_with_libs(args.include_executable,
    File "./tests/docker/docker.py", line 149, in _copy_binary_with_libs
      libs = _get_so_libs(src)
    File "./tests/docker/docker.py", line 123, in _get_so_libs
      libs.append(s.group(1))
  NameError: name 's' is not defined

Fixes: 5e33f7fead5 ("tests/docker: better handle symlinked libs")
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20210119050149.516910-1-f4bug@amsat.org>
---
 tests/docker/docker.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index 884dfeb29c..0b4f6167b3 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -120,7 +120,7 @@ def _get_so_libs(executable):
             search = ldd_re.search(line)
             if search:
                 try:
-                    libs.append(s.group(1))
+                    libs.append(search.group(1))
                 except IndexError:
                     pass
     except subprocess.CalledProcessError:
-- 
2.20.1

[PATCH v2 2/8] tests/docker: Fix typo in help message

[Alex Bennée]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

To have the variable properly passed, we need to set it,
ie. NOUSER=1. Fix the message displayed by 'make docker'.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20210119052120.522069-1-f4bug@amsat.org>
---
 tests/docker/Makefile.include | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 0779dab5b9..bdc53ddfcf 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -209,7 +209,7 @@ endif
 	@echo '                         before running the command.'
 	@echo '    NETWORK=1            Enable virtual network interface with default backend.'
 	@echo '    NETWORK=$$BACKEND     Enable virtual network interface with $$BACKEND.'
-	@echo '    NOUSER               Define to disable adding current user to containers passwd.'
+	@echo '    NOUSER=1             Define to disable adding current user to containers passwd.'
 	@echo '    NOCACHE=1            Ignore cache when build images.'
 	@echo '    EXECUTABLE=<path>    Include executable in image.'
 	@echo '    EXTRA_FILES="<path> [... <path>]"'
-- 
2.20.1

[PATCH v2 3/8] tests/docker: make _copy_with_mkdir accept missing files

[Alex Bennée]

Depending on the linker/ldd setup we might get a file with no path.
Typically this is the psuedo library linux-vdso.so which doesn't
actually exist on the disk. Rather than try and catch these distro
specific edge cases just shout about it and try and continue.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/docker/docker.py | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index 0b4f6167b3..fb3de41c0b 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -103,7 +103,12 @@ def _copy_with_mkdir(src, root_dir, sub_path='.'):
         pass
 
     dest_file = "%s/%s" % (dest_dir, os.path.basename(src))
-    copy(src, dest_file)
+
+    try:
+        copy(src, dest_file)
+    except FileNotFoundError:
+        print("Couldn't copy %s to %s" % (src, dest_file))
+        pass
 
 
 def _get_so_libs(executable):
-- 
2.20.1

[PATCH v2 4/8] tests/docker: preserve original name when copying libs

[Alex Bennée]

While it is important we chase down the symlinks to copy the correct
data we can confuse the kernel by renaming the interpreter to what is
in the binary. Extend _copy_with_mkdir to preserve the original name
of the file when asked.

Fixes: 5e33f7fead ("tests/docker: better handle symlinked libs")
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/docker/docker.py | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index fb3de41c0b..39da3fefcf 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -93,7 +93,7 @@ def _guess_engine_command():
                     commands_txt)
 
 
-def _copy_with_mkdir(src, root_dir, sub_path='.'):
+def _copy_with_mkdir(src, root_dir, sub_path='.', name=None):
     """Copy src into root_dir, creating sub_path as needed."""
     dest_dir = os.path.normpath("%s/%s" % (root_dir, sub_path))
     try:
@@ -102,7 +102,7 @@ def _copy_with_mkdir(src, root_dir, sub_path='.'):
         # we can safely ignore already created directories
         pass
 
-    dest_file = "%s/%s" % (dest_dir, os.path.basename(src))
+    dest_file = "%s/%s" % (dest_dir, name if name else os.path.basename(src))
 
     try:
         copy(src, dest_file)
@@ -155,8 +155,9 @@ def _copy_binary_with_libs(src, bin_dest, dest_dir):
     if libs:
         for l in libs:
             so_path = os.path.dirname(l)
+            name = os.path.basename(l)
             real_l = os.path.realpath(l)
-            _copy_with_mkdir(real_l, dest_dir, so_path)
+            _copy_with_mkdir(real_l, dest_dir, so_path, name)
 
 
 def _check_binfmt_misc(executable):
-- 
2.20.1

[PATCH v2 5/8] tests/docker: alias docker-help target for consistency

[Alex Bennée]

We have a bunch of -help targets so this will save some cognitive
dissonance. Keep the original for those with muscle memory.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/docker/Makefile.include | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index bdc53ddfcf..a5c1e4a615 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -1,6 +1,6 @@
 # Makefile for Docker tests
 
-.PHONY: docker docker-test docker-clean docker-image docker-qemu-src
+.PHONY: docker docker-help docker-test docker-clean docker-image docker-qemu-src
 
 NULL :=
 SPACE := $(NULL) #
@@ -218,6 +218,8 @@ endif
 	@echo '                         Specify which container engine to run.'
 	@echo '    REGISTRY=url         Cache builds from registry (default:$(DOCKER_REGISTRY))'
 
+docker-help: docker
+
 # This rule if for directly running against an arbitrary docker target.
 # It is called by the expanded docker targets (e.g. make
 # docker-test-foo@bar) which will do additional verification.
-- 
2.20.1

[PATCH v2 6/8] tests/docker: add a docker-exec-copy-test

[Alex Bennée]

This provides test machinery for checking the QEMU copying logic works
properly. It takes considerably less time to run than starting a
debootstrap only for it to fail later. I considered adding a remove
command to docker.py but figured that might be gold plating given the
relative size of the containers compared to the ones with actual stuff
in them.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/docker/Makefile.include         | 20 +++++++++++++++++++-
 tests/docker/docker.py                |  7 ++++++-
 tests/docker/dockerfiles/empty.docker |  8 ++++++++
 3 files changed, 33 insertions(+), 2 deletions(-)
 create mode 100644 tests/docker/dockerfiles/empty.docker

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index a5c1e4a615..93b29ad823 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -11,7 +11,7 @@ HOST_ARCH = $(if $(ARCH),$(ARCH),$(shell uname -m))
 DOCKER_SUFFIX := .docker
 DOCKER_FILES_DIR := $(SRC_PATH)/tests/docker/dockerfiles
 # we don't run tests on intermediate images (used as base by another image)
-DOCKER_PARTIAL_IMAGES := debian10 debian11 debian-bootstrap
+DOCKER_PARTIAL_IMAGES := debian10 debian11 debian-bootstrap empty
 DOCKER_IMAGES := $(sort $(notdir $(basename $(wildcard $(DOCKER_FILES_DIR)/*.docker))))
 DOCKER_TARGETS := $(patsubst %,docker-image-%,$(DOCKER_IMAGES))
 # Use a global constant ccache directory to speed up repetitive builds
@@ -92,6 +92,24 @@ docker-binfmt-image-debian-%: $(DOCKER_FILES_DIR)/debian-bootstrap.docker
 			{ echo "You will need to build $(EXECUTABLE)"; exit 1;},\
 			"CHECK", "debian-$* exists"))
 
+# These are test targets
+USER_TCG_TARGETS=$(patsubst %-linux-user,qemu-%,$(filter %-linux-user,$(TARGET_DIRS)))
+EXEC_COPY_TESTS=$(patsubst %,docker-exec-copy-test-%, $(USER_TCG_TARGETS))
+
+$(EXEC_COPY_TESTS): docker-exec-copy-test-%: $(DOCKER_FILES_DIR)/empty.docker
+	$(call quiet-command,							\
+		$(DOCKER_SCRIPT) build -t qemu/exec-copy-test-$* -f $< 		\
+			$(if $V,,--quiet) --no-cache 				\
+			--include-executable=$*					\
+			--skip-binfmt,						\
+			"TEST","copy $* to container")
+	$(call quiet-command,							\
+		$(DOCKER_SCRIPT) run qemu/exec-copy-test-$* 			\
+			/$* -version > tests/docker-exec-copy-test-$*.out,	\
+			"TEST","check $* works in container")
+
+docker-exec-copy-test: $(EXEC_COPY_TESTS)
+
 endif
 
 # Enforce dependencies for composite images
diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index 39da3fefcf..d28df4c140 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -438,6 +438,9 @@ class BuildCommand(SubCommand):
                             help="""Specify a binary that will be copied to the
                             container together with all its dependent
                             libraries""")
+        parser.add_argument("--skip-binfmt",
+                            action="store_true",
+                            help="""Skip binfmt entry check (used for testing)""")
         parser.add_argument("--extra-files", nargs='*',
                             help="""Specify files that will be copied in the
                             Docker image, fulfilling the ADD directive from the
@@ -466,7 +469,9 @@ class BuildCommand(SubCommand):
             docker_dir = tempfile.mkdtemp(prefix="docker_build")
 
             # Validate binfmt_misc will work
-            if args.include_executable:
+            if args.skip_binfmt:
+                qpath = args.include_executable
+            elif args.include_executable:
                 qpath, enabled = _check_binfmt_misc(args.include_executable)
                 if not enabled:
                     return 1
diff --git a/tests/docker/dockerfiles/empty.docker b/tests/docker/dockerfiles/empty.docker
new file mode 100644
index 0000000000..9ba980f1a8
--- /dev/null
+++ b/tests/docker/dockerfiles/empty.docker
@@ -0,0 +1,8 @@
+#
+# Empty Dockerfile
+#
+
+FROM scratch
+
+# Add everything from the context into the container
+ADD . /
-- 
2.20.1

[PATCH v2 7/8] configure: make version_ge more tolerant of shady version input

[Alex Bennée]

When checking GDB versions we have to tolerate all sorts of random
distro extensions to the version string. While we already attempt to
do some of that before we call version_ge is makes sense to try and
regularise the first input by stripping extraneous -'s. While we at it
convert the old-style shell quoting into a cleaner form t shut up my
editors linter lest it confuse me by underlining the whole line.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 configure | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 6f6a319c2f..dd99939b34 100755
--- a/configure
+++ b/configure
@@ -198,8 +198,8 @@ has() {
 }
 
 version_ge () {
-    local_ver1=`echo $1 | tr . ' '`
-    local_ver2=`echo $2 | tr . ' '`
+    local_ver1=$(expr "$1" : '\([0-9.]*\)' | tr . ' ')
+    local_ver2=$(echo "$2" | tr . ' ')
     while true; do
         set x $local_ver1
         local_first=${2-0}
-- 
2.20.1

[PATCH v2 8/8] configure: bump the minimum gdb version for check-tcg to 9.1

[Alex Bennée]

For SVE, currently the bulk of the GDB TCG tests, we need at least GDB
9.1 to support the "ieee_half" data type we report. This only affects
when GDB tests are run; users can still use lower versions of gdb as
long as they aren't talking to an SVE enabled model. The work around
is to either get a newer gdb or disable SVE for their CPU model.

Reported-by: Claudio Fontana <cfontana@suse.de>
Cc: Luis Machado <luis.machado@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index dd99939b34..0cf9718d4f 100755
--- a/configure
+++ b/configure
@@ -6169,7 +6169,7 @@ fi
 
 if test -n "$gdb_bin"; then
     gdb_version=$($gdb_bin --version | head -n 1)
-    if version_ge ${gdb_version##* } 8.3.1; then
+    if version_ge ${gdb_version##* } 9.1; then
         echo "HAVE_GDB_BIN=$gdb_bin" >> $config_host_mak
     fi
 fi
-- 
2.20.1

Re: [PATCH v2 0/8] testing/next (docker, binfmt, gdb version)

[Alex Bennée]

Alex Bennée <alex.bennee@linaro.org> writes:

> Hi,
>
> The testing updates keep on rolling. This expands on the last version
> by adding some more handling of weird gdb version numbers and bumping
> the minimum requirement again.
>
> The following need review:
>
>  - configure: bump the minimum gdb version for check-tcg to 9.1
>  - configure: make version_ge more tolerant of shady version input
>  - tests/docker: add a docker-exec-copy-test
>  - tests/docker: alias docker-help target for consistency
>  - tests/docker: preserve original name when copying libs

Ping?


-- 
Alex Bennée

Re: [PATCH v2 7/8] configure: make version_ge more tolerant of shady version input

[Thomas Huth]

On 22/01/2021 19.18, Alex Bennée wrote:
> When checking GDB versions we have to tolerate all sorts of random
> distro extensions to the version string. While we already attempt to
> do some of that before we call version_ge is makes sense to try and
> regularise the first input by stripping extraneous -'s. While we at it
> convert the old-style shell quoting into a cleaner form t shut up my
> editors linter lest it confuse me by underlining the whole line.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   configure | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/configure b/configure
> index 6f6a319c2f..dd99939b34 100755
> --- a/configure
> +++ b/configure
> @@ -198,8 +198,8 @@ has() {
>   }
>   
>   version_ge () {
> -    local_ver1=`echo $1 | tr . ' '`
> -    local_ver2=`echo $2 | tr . ' '`
> +    local_ver1=$(expr "$1" : '\([0-9.]*\)' | tr . ' ')
> +    local_ver2=$(echo "$2" | tr . ' ')
>       while true; do
>           set x $local_ver1
>           local_first=${2-0}
> 

This silences the warnings that I've recently seen when running configure.

Tested-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v2 5/8] tests/docker: alias docker-help target for consistency

[Thomas Huth]

On 22/01/2021 19.18, Alex Bennée wrote:
> We have a bunch of -help targets so this will save some cognitive
> dissonance. Keep the original for those with muscle memory.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   tests/docker/Makefile.include | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
> index bdc53ddfcf..a5c1e4a615 100644
> --- a/tests/docker/Makefile.include
> +++ b/tests/docker/Makefile.include
> @@ -1,6 +1,6 @@
>   # Makefile for Docker tests
>   
> -.PHONY: docker docker-test docker-clean docker-image docker-qemu-src
> +.PHONY: docker docker-help docker-test docker-clean docker-image docker-qemu-src
>   
>   NULL :=
>   SPACE := $(NULL) #
> @@ -218,6 +218,8 @@ endif
>   	@echo '                         Specify which container engine to run.'
>   	@echo '    REGISTRY=url         Cache builds from registry (default:$(DOCKER_REGISTRY))'
>   
> +docker-help: docker
> +
>   # This rule if for directly running against an arbitrary docker target.
>   # It is called by the expanded docker targets (e.g. make
>   # docker-test-foo@bar) which will do additional verification.
> 

Do you also want to rename it in the main Makefile:

  $(call print-help,docker,Help about targets running tests inside containers)

?

Anyway:
Reviewed-by: Thomas Huth <thuth@redhat.com>