[PATCH 0/8] run-coverity-scan: misc improvements, especially for docker mode

[PATCH 0/8] run-coverity-scan: misc improvements, especially for docker mode

[Paolo Bonzini]

These include:

1) podman support through tests/docker/docker.py

2) avoiding repeated downloading of the tools in the container, by
   sharing the cache with the host

3) support for --update-tools-only --docker (though unlike regular
   --update-tools-only it must be run from within a QEMU source tree)

4) not related to docker mode, but used by it, a new --no-update-tools
   option that does not check for tool updates

5) the ability to get the Coverity token from git configuration, and
   also to have email from git configuration if it is not equal to
   user.email.

Patches 1 and 2 are tweaks to tests/docker/docker.py, while the others
are for run-coverity-scan.

Paolo Bonzini (8):
  docker.py/build: support -t and -f arguments
  docker.py/build: support binary files in --extra-files
  run-coverity-scan: get Coverity token and email from special git
    config section
  run-coverity-scan: use docker.py
  run-coverity-scan: add --no-update-tools option
  run-coverity-scan: use --no-update-tools in docker run
  run-coverity-scan: download tools outside the container
  run-coverity-scan: support --update-tools-only --docker

 scripts/coverity-scan/coverity-scan.docker |   3 +-
 scripts/coverity-scan/run-coverity-scan    | 126 +++++++++++++--------
 tests/docker/Makefile.include              |   2 +-
 tests/docker/docker.py                     |  14 ++-
 4 files changed, 88 insertions(+), 57 deletions(-)

-- 
2.18.2

[PATCH 1/8] docker.py/build: support -t and -f arguments

[Paolo Bonzini]

The docker.py command line is subtly different from docker and podman's,
in that the tag and Dockerfile are passed via positional arguments.
Remove this gratuitous difference and just parse -f and -t.

-f was previously used by --extra-files, only keep the long option.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/docker/Makefile.include | 2 +-
 tests/docker/docker.py        | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 43a8678688..262704663f 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -55,7 +55,7 @@ docker-image-%: $(DOCKER_FILES_DIR)/%.docker
 else
 docker-image-%: $(DOCKER_FILES_DIR)/%.docker
 	$(call quiet-command,\
-		$(DOCKER_SCRIPT) build qemu:$* $< \
+		$(DOCKER_SCRIPT) build -t qemu:$* -f $< \
 		$(if $V,,--quiet) $(if $(NOCACHE),--no-cache) \
 		$(if $(NOUSER),,--add-current-user) \
 		$(if $(EXTRA_FILES),--extra-files $(EXTRA_FILES))\
diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index d8268c1111..ad61747bae 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -391,16 +391,16 @@ class BuildCommand(SubCommand):
                             help="""Specify a binary that will be copied to the
                             container together with all its dependent
                             libraries""")
-        parser.add_argument("--extra-files", "-f", nargs='*',
+        parser.add_argument("--extra-files", nargs='*',
                             help="""Specify files that will be copied in the
                             Docker image, fulfilling the ADD directive from the
                             Dockerfile""")
         parser.add_argument("--add-current-user", "-u", dest="user",
                             action="store_true",
                             help="Add the current user to image's passwd")
-        parser.add_argument("tag",
+        parser.add_argument("-t", dest="tag",
                             help="Image Tag")
-        parser.add_argument("dockerfile",
+        parser.add_argument("-f", dest="dockerfile",
                             help="Dockerfile name")
 
     def run(self, args, argv):
-- 
2.18.2

Re: [PATCH 1/8] docker.py/build: support -t and -f arguments

[Philippe Mathieu-Daudé]

On 4/22/20 7:23 PM, Paolo Bonzini wrote:
> The docker.py command line is subtly different from docker and podman's,
> in that the tag and Dockerfile are passed via positional arguments.
> Remove this gratuitous difference and just parse -f and -t.
> 
> -f was previously used by --extra-files, only keep the long option.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   tests/docker/Makefile.include | 2 +-
>   tests/docker/docker.py        | 6 +++---
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
> index 43a8678688..262704663f 100644
> --- a/tests/docker/Makefile.include
> +++ b/tests/docker/Makefile.include
> @@ -55,7 +55,7 @@ docker-image-%: $(DOCKER_FILES_DIR)/%.docker
>   else
>   docker-image-%: $(DOCKER_FILES_DIR)/%.docker
>   	$(call quiet-command,\
> -		$(DOCKER_SCRIPT) build qemu:$* $< \
> +		$(DOCKER_SCRIPT) build -t qemu:$* -f $< \
>   		$(if $V,,--quiet) $(if $(NOCACHE),--no-cache) \
>   		$(if $(NOUSER),,--add-current-user) \
>   		$(if $(EXTRA_FILES),--extra-files $(EXTRA_FILES))\
> diff --git a/tests/docker/docker.py b/tests/docker/docker.py
> index d8268c1111..ad61747bae 100755
> --- a/tests/docker/docker.py
> +++ b/tests/docker/docker.py
> @@ -391,16 +391,16 @@ class BuildCommand(SubCommand):
>                               help="""Specify a binary that will be copied to the
>                               container together with all its dependent
>                               libraries""")
> -        parser.add_argument("--extra-files", "-f", nargs='*',
> +        parser.add_argument("--extra-files", nargs='*',
>                               help="""Specify files that will be copied in the
>                               Docker image, fulfilling the ADD directive from the
>                               Dockerfile""")
>           parser.add_argument("--add-current-user", "-u", dest="user",
>                               action="store_true",
>                               help="Add the current user to image's passwd")
> -        parser.add_argument("tag",
> +        parser.add_argument("-t", dest="tag",
>                               help="Image Tag")
> -        parser.add_argument("dockerfile",
> +        parser.add_argument("-f", dest="dockerfile",
>                               help="Dockerfile name")
>   
>       def run(self, args, argv):
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH 2/8] docker.py/build: support binary files in --extra-files

[Paolo Bonzini]

Read the --extra-files in binary mode to avoid encoding errors.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/docker/docker.py | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index ad61747bae..85e1dda10f 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -56,15 +56,19 @@ class EngineEnum(enum.IntEnum):
 
 USE_ENGINE = EngineEnum.AUTO
 
+def _bytes_checksum(bytes):
+    """Calculate a digest string unique to the text content"""
+    return hashlib.sha1(bytes).hexdigest()
+
 def _text_checksum(text):
     """Calculate a digest string unique to the text content"""
-    return hashlib.sha1(text.encode('utf-8')).hexdigest()
+    return _bytes_checksum(text.encode('utf-8'))
 
 def _read_dockerfile(path):
     return open(path, 'rt', encoding='utf-8').read()
 
 def _file_checksum(filename):
-    return _text_checksum(_read_dockerfile(filename))
+    return _bytes_checksum(open(filename, 'rb').read())
 
 
 def _guess_engine_command():
-- 
2.18.2

Re: [PATCH 2/8] docker.py/build: support binary files in --extra-files

[Philippe Mathieu-Daudé]

On 4/22/20 7:23 PM, Paolo Bonzini wrote:
> Read the --extra-files in binary mode to avoid encoding errors.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   tests/docker/docker.py | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/docker/docker.py b/tests/docker/docker.py
> index ad61747bae..85e1dda10f 100755
> --- a/tests/docker/docker.py
> +++ b/tests/docker/docker.py
> @@ -56,15 +56,19 @@ class EngineEnum(enum.IntEnum):
>   
>   USE_ENGINE = EngineEnum.AUTO
>   
> +def _bytes_checksum(bytes):
> +    """Calculate a digest string unique to the text content"""
> +    return hashlib.sha1(bytes).hexdigest()
> +
>   def _text_checksum(text):
>       """Calculate a digest string unique to the text content"""
> -    return hashlib.sha1(text.encode('utf-8')).hexdigest()
> +    return _bytes_checksum(text.encode('utf-8'))
>   
>   def _read_dockerfile(path):
>       return open(path, 'rt', encoding='utf-8').read()
>   
>   def _file_checksum(filename):
> -    return _text_checksum(_read_dockerfile(filename))
> +    return _bytes_checksum(open(filename, 'rb').read())
>   
>   
>   def _guess_engine_command():
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH 3/8] run-coverity-scan: get Coverity token and email from special git config section

[Paolo Bonzini]

Support a [coverity] section in .git/config.  It can be used to retrieve the
token and also, if it is different from user.email, the username of the
submitter.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/coverity-scan/run-coverity-scan | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/scripts/coverity-scan/run-coverity-scan b/scripts/coverity-scan/run-coverity-scan
index 2e067ef5cf..f7325b570c 100755
--- a/scripts/coverity-scan/run-coverity-scan
+++ b/scripts/coverity-scan/run-coverity-scan
@@ -41,9 +41,10 @@
 #                   is intended mainly for internal use by the Docker support
 #
 # User-specifiable environment variables:
-#  COVERITY_TOKEN -- Coverity token
+#  COVERITY_TOKEN -- Coverity token (default: looks at your
+#                    coverity.token config)
 #  COVERITY_EMAIL -- the email address to use for uploads (default:
-#                    looks at your git user.email config)
+#                    looks at your git coverity.email or user.email config)
 #  COVERITY_BUILD_CMD -- make command (default: 'make -jN' where N is
 #                    number of CPUs as determined by 'nproc')
 #  COVERITY_TOOL_BASE -- set to directory to put coverity tools
@@ -62,7 +63,7 @@ check_upload_permissions() {
 
     echo "Checking upload permissions..."
 
-    if ! up_perm="$(wget https://scan.coverity.com/api/upload_permitted --post-data "token=$PROJTOKEN&project=$PROJNAME" -q -O -)"; then
+    if ! up_perm="$(wget https://scan.coverity.com/api/upload_permitted --post-data "token=$COVERITY_TOKEN&project=$PROJNAME" -q -O -)"; then
         echo "Coverity Scan API access denied: bad token?"
         exit 1
     fi
@@ -100,14 +101,14 @@ update_coverity_tools () {
     cd "$COVERITY_TOOL_BASE"
 
     echo "Checking for new version of coverity build tools..."
-    wget https://scan.coverity.com/download/linux64 --post-data "token=$PROJTOKEN&project=$PROJNAME&md5=1" -O coverity_tool.md5.new
+    wget https://scan.coverity.com/download/linux64 --post-data "token=$COVERITY_TOKEN&project=$PROJNAME&md5=1" -O coverity_tool.md5.new
 
     if ! cmp -s coverity_tool.md5 coverity_tool.md5.new; then
         # out of date md5 or no md5: download new build tool
         # blow away the old build tool
         echo "Downloading coverity build tools..."
         rm -rf coverity_tool coverity_tool.tgz
-        wget https://scan.coverity.com/download/linux64 --post-data "token=$PROJTOKEN&project=$PROJNAME" -O coverity_tool.tgz
+        wget https://scan.coverity.com/download/linux64 --post-data "token=$COVERITY_TOKEN&project=$PROJNAME" -O coverity_tool.tgz
         if ! (cat coverity_tool.md5.new; echo "  coverity_tool.tgz") | md5sum -c --status; then
             echo "Downloaded tarball didn't match md5sum!"
             exit 1
@@ -205,6 +206,9 @@ while [ "$#" -ge 1 ]; do
     esac
 done
 
+if [ -z "$COVERITY_TOKEN" ]; then
+    COVERITY_TOKEN="$(git config coverity.token)"
+fi
 if [ -z "$COVERITY_TOKEN" ]; then
     echo "COVERITY_TOKEN environment variable not set"
     exit 1
@@ -225,7 +229,6 @@ if [ -z "$SRCDIR" ]; then
     SRCDIR="$PWD"
 fi
 
-PROJTOKEN="$COVERITY_TOKEN"
 PROJNAME=QEMU
 TARBALL=cov-int.tar.xz
 
@@ -268,6 +271,9 @@ if [ -z "$DESCRIPTION" ]; then
     DESCRIPTION="$(git rev-parse HEAD)"
 fi
 
+if [ -z "$COVERITY_EMAIL" ]; then
+    COVERITY_EMAIL="$(git config coverity.email)"
+fi
 if [ -z "$COVERITY_EMAIL" ]; then
     COVERITY_EMAIL="$(git config user.email)"
 fi
@@ -393,7 +399,7 @@ if [ "$DRYRUN" = yes ]; then
     exit 0
 fi
 
-curl --form token="$PROJTOKEN" --form email="$COVERITY_EMAIL" \
+curl --form token="$COVERITY_TOKEN" --form email="$COVERITY_EMAIL" \
      --form file=@"$TARBALL" --form version="$VERSION" \
      --form description="$DESCRIPTION" \
      https://scan.coverity.com/builds?project="$PROJNAME"
-- 
2.18.2

Re: [PATCH 3/8] run-coverity-scan: get Coverity token and email from special git config section

[Peter Maydell]

On Wed, 22 Apr 2020 at 18:24, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Support a [coverity] section in .git/config.  It can be used to retrieve the
> token and also, if it is different from user.email, the username of the
> submitter.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I think a couple of references to PROJTOKEN in comments need
updating to say COVERITY_TOKEN. Otherwise

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 4/8] run-coverity-scan: use docker.py

[Paolo Bonzini]

Our trusted docker wrapper allows run-coverity-scan to run with both
docker and podman.

For the "run" phase this is transparent; for the "build" phase however
scripts are replaced with a bind mount (-v).  This is not an issue
because the secret option is meant for secrets stored globally in the
system and bind mounts are a valid substitute for secrets that are known
to whoever builds the container.

This also removes the need for DOCKER_BUILDKIT=1.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	Later in the series, the secret will not be used in "docker build"
	at all.

 scripts/coverity-scan/coverity-scan.docker |  2 +-
 scripts/coverity-scan/run-coverity-scan    | 23 ++++++++++++----------
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/scripts/coverity-scan/coverity-scan.docker b/scripts/coverity-scan/coverity-scan.docker
index a4f64d1283..6f0460b66c 100644
--- a/scripts/coverity-scan/coverity-scan.docker
+++ b/scripts/coverity-scan/coverity-scan.docker
@@ -128,4 +128,4 @@ RUN rpm -q $PACKAGES | sort > /packages.txt
 ENV PATH $PATH:/usr/libexec/python3-sphinx/
 ENV COVERITY_TOOL_BASE=/coverity-tools
 COPY run-coverity-scan run-coverity-scan
-RUN --mount=type=secret,id=coverity.token,required ./run-coverity-scan --update-tools-only --tokenfile /run/secrets/coverity.token
+RUN ./run-coverity-scan --update-tools-only --tokenfile /work/token
diff --git a/scripts/coverity-scan/run-coverity-scan b/scripts/coverity-scan/run-coverity-scan
index f7325b570c..ae1fc7ae76 100755
--- a/scripts/coverity-scan/run-coverity-scan
+++ b/scripts/coverity-scan/run-coverity-scan
@@ -197,6 +197,12 @@ while [ "$#" -ge 1 ]; do
             ;;
         --docker)
             DOCKER=yes
+            DOCKER_ENGINE=auto
+            shift
+            ;;
+        --docker=*)
+            DOCKER=yes
+            DOCKER_ENGINE=${1#--docker=}
             shift
             ;;
         *)
@@ -283,9 +289,8 @@ if [ "$DOCKER" = yes ]; then
     # build docker container including the coverity-scan tools
     # Put the Coverity token into a temporary file that only
     # we have read access to, and then pass it to docker build
-    # using --secret. This requires at least Docker 18.09.
-    # Mostly what we are trying to do here is ensure we don't leak
-    # the token into the Docker image.
+    # using a volume.  A volume is enough for the token not to
+    # leak into the Docker image.
     umask 077
     SECRETDIR=$(mktemp -d)
     if [ -z "$SECRETDIR" ]; then
@@ -300,12 +305,10 @@ if [ "$DOCKER" = yes ]; then
     # TODO: This re-downloads the tools every time, rather than
     # caching and reusing the image produced with the downloaded tools.
     # Not sure why.
-    # TODO: how do you get 'docker build' to print the output of the
-    # commands it is running to its stdout? This would be useful for debug.
-    DOCKER_BUILDKIT=1 docker build -t coverity-scanner \
-                   --secret id=coverity.token,src="$SECRET" \
-                   -f scripts/coverity-scan/coverity-scan.docker \
-                   scripts/coverity-scan
+    tests/docker/docker.py --engine ${DOCKER_ENGINE} build \
+                   -t coverity-scanner -f scripts/coverity-scan/coverity-scan.docker \
+                   -v "$SECRETDIR:/work" \
+                   --extra-files scripts/coverity-scan/run-coverity-scan
     echo "Archiving sources to be analyzed..."
     ./scripts/archive-source.sh "$SECRETDIR/qemu-sources.tgz"
     if [ "$DRYRUN" = yes ]; then
@@ -323,7 +326,7 @@ if [ "$DOCKER" = yes ]; then
     # Arrange for this docker run to get access to the sources with -v.
     # We pass through all the configuration from the outer script to the inner.
     export COVERITY_EMAIL COVERITY_BUILD_CMD
-    docker run -it --env COVERITY_EMAIL --env COVERITY_BUILD_CMD \
+    tests/docker/docker.py run -it --env COVERITY_EMAIL --env COVERITY_BUILD_CMD \
            -v "$SECRETDIR:/work" coverity-scanner \
            ./run-coverity-scan --version "$VERSION" \
            --description "$DESCRIPTION" $DRYRUNARG --tokenfile /work/token \
-- 
2.18.2

Re: [PATCH 4/8] run-coverity-scan: use docker.py

[Peter Maydell]

On Wed, 22 Apr 2020 at 18:24, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Our trusted docker wrapper allows run-coverity-scan to run with both
> docker and podman.
>
> For the "run" phase this is transparent; for the "build" phase however
> scripts are replaced with a bind mount (-v).  This is not an issue
> because the secret option is meant for secrets stored globally in the
> system and bind mounts are a valid substitute for secrets that are known
> to whoever builds the container.
>
> This also removes the need for DOCKER_BUILDKIT=1.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

> --- a/scripts/coverity-scan/run-coverity-scan
> +++ b/scripts/coverity-scan/run-coverity-scan
> @@ -197,6 +197,12 @@ while [ "$#" -ge 1 ]; do
>              ;;
>          --docker)
>              DOCKER=yes
> +            DOCKER_ENGINE=auto
> +            shift
> +            ;;
> +        --docker=*)
> +            DOCKER=yes
> +            DOCKER_ENGINE=${1#--docker=}
>              shift

The comment at the top of the file documenting the command line
options needs updating.

We don't accept --foo=bar for anything else: options either take
no argument, or take an argument as a following (ie space separated)
parameter. It would be more consistent with that to have
"--docker-engine foo" as a separate option from "--docker".

Otherwise looks OK.

thanks
-- PMM

Re: [PATCH 4/8] run-coverity-scan: use docker.py

[Paolo Bonzini]

On 27/04/20 14:42, Peter Maydell wrote:
> The comment at the top of the file documenting the command line
> options needs updating.
> 
> We don't accept --foo=bar for anything else: options either take
> no argument, or take an argument as a following (ie space separated)
> parameter. It would be more consistent with that to have
> "--docker-engine foo" as a separate option from "--docker".

Sure.

Paolo

[PATCH 5/8] run-coverity-scan: add --no-update-tools option

[Paolo Bonzini]

Provide a quick way to skip building the container while we figure out how
to get caching right.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/coverity-scan/run-coverity-scan | 37 +++++++++++++++----------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/scripts/coverity-scan/run-coverity-scan b/scripts/coverity-scan/run-coverity-scan
index ae1fc7ae76..9403429849 100755
--- a/scripts/coverity-scan/run-coverity-scan
+++ b/scripts/coverity-scan/run-coverity-scan
@@ -31,6 +31,7 @@
 #   --dry-run : run the tools, but don't actually do the upload
 #   --docker : create and work inside a docker container
 #   --update-tools-only : update the cached copy of the tools, but don't run them
+#   --no-update-tools : do not update the cached copy of the tools
 #   --tokenfile : file to read Coverity token from
 #   --version ver : specify version being analyzed (default: ask git)
 #   --description desc : specify description of this version (default: ask git)
@@ -128,7 +129,7 @@ update_coverity_tools () {
 
 # Check user-provided environment variables and arguments
 DRYRUN=no
-UPDATE_ONLY=no
+UPDATE=yes
 DOCKER=no
 
 while [ "$#" -ge 1 ]; do
@@ -137,9 +138,13 @@ while [ "$#" -ge 1 ]; do
             shift
             DRYRUN=yes
             ;;
+        --no-update-tools)
+            shift
+            UPDATE=no
+            ;;
         --update-tools-only)
             shift
-            UPDATE_ONLY=yes
+            UPDATE=only
             ;;
         --version)
             shift
@@ -238,12 +243,12 @@ fi
 PROJNAME=QEMU
 TARBALL=cov-int.tar.xz
 
-if [ "$UPDATE_ONLY" = yes ] && [ "$DOCKER" = yes ]; then
+if [ "$UPDATE" = only ] && [ "$DOCKER" = yes ]; then
     echo "Combining --docker and --update-only is not supported"
     exit 1
 fi
 
-if [ "$UPDATE_ONLY" = yes ]; then
+if [ "$UPDATE" = only ]; then
     # Just do the tools update; we don't need to check whether
     # we are in a source tree or have upload rights for this,
     # so do it before some of the command line and source tree checks.
@@ -286,7 +291,6 @@ fi
 
 # Run ourselves inside docker if that's what the user wants
 if [ "$DOCKER" = yes ]; then
-    # build docker container including the coverity-scan tools
     # Put the Coverity token into a temporary file that only
     # we have read access to, and then pass it to docker build
     # using a volume.  A volume is enough for the token not to
@@ -301,14 +305,17 @@ if [ "$DOCKER" = yes ]; then
     echo "Created temporary directory $SECRETDIR"
     SECRET="$SECRETDIR/token"
     echo "$COVERITY_TOKEN" > "$SECRET"
-    echo "Building docker container..."
-    # TODO: This re-downloads the tools every time, rather than
-    # caching and reusing the image produced with the downloaded tools.
-    # Not sure why.
-    tests/docker/docker.py --engine ${DOCKER_ENGINE} build \
-                   -t coverity-scanner -f scripts/coverity-scan/coverity-scan.docker \
-                   -v "$SECRETDIR:/work" \
-                   --extra-files scripts/coverity-scan/run-coverity-scan
+    if [ "$UPDATE" != no ]; then
+        # build docker container including the coverity-scan tools
+        echo "Building docker container..."
+        # TODO: This re-downloads the tools every time, rather than
+        # caching and reusing the image produced with the downloaded tools.
+        # Not sure why.
+        tests/docker/docker.py --engine ${DOCKER_ENGINE} build \
+                       -t coverity-scanner -f scripts/coverity-scan/coverity-scan.docker \
+                       -v "$SECRETDIR:/work" \
+                       --extra-files scripts/coverity-scan/run-coverity-scan
+    fi
     echo "Archiving sources to be analyzed..."
     ./scripts/archive-source.sh "$SECRETDIR/qemu-sources.tgz"
     if [ "$DRYRUN" = yes ]; then
@@ -343,7 +350,9 @@ fi
 
 check_upload_permissions
 
-update_coverity_tools
+if [ "$UPDATE" != no ]; then
+    update_coverity_tools
+fi
 
 TOOLBIN="$(cd "$COVERITY_TOOL_BASE" && echo $PWD/coverity_tool/cov-analysis-*/bin)"
 
-- 
2.18.2

Re: [PATCH 5/8] run-coverity-scan: add --no-update-tools option

[Peter Maydell]

On Wed, 22 Apr 2020 at 18:24, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Provide a quick way to skip building the container while we figure out how
> to get caching right.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  scripts/coverity-scan/run-coverity-scan | 37 +++++++++++++++----------
>  1 file changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/scripts/coverity-scan/run-coverity-scan b/scripts/coverity-scan/run-coverity-scan
> index ae1fc7ae76..9403429849 100755
> --- a/scripts/coverity-scan/run-coverity-scan
> +++ b/scripts/coverity-scan/run-coverity-scan
> @@ -31,6 +31,7 @@
>  #   --dry-run : run the tools, but don't actually do the upload
>  #   --docker : create and work inside a docker container
>  #   --update-tools-only : update the cached copy of the tools, but don't run them
> +#   --no-update-tools : do not update the cached copy of the tools
>  #   --tokenfile : file to read Coverity token from
>  #   --version ver : specify version being analyzed (default: ask git)
>  #   --description desc : specify description of this version (default: ask git)
> @@ -128,7 +129,7 @@ update_coverity_tools () {
>
>  # Check user-provided environment variables and arguments
>  DRYRUN=no
> -UPDATE_ONLY=no
> +UPDATE=yes
>  DOCKER=no
>
>  while [ "$#" -ge 1 ]; do
> @@ -137,9 +138,13 @@ while [ "$#" -ge 1 ]; do
>              shift
>              DRYRUN=yes
>              ;;
> +        --no-update-tools)
> +            shift
> +            UPDATE=no
> +            ;;
>          --update-tools-only)
>              shift
> -            UPDATE_ONLY=yes
> +            UPDATE=only
>              ;;
>          --version)
>              shift
> @@ -238,12 +243,12 @@ fi
>  PROJNAME=QEMU
>  TARBALL=cov-int.tar.xz
>
> -if [ "$UPDATE_ONLY" = yes ] && [ "$DOCKER" = yes ]; then
> +if [ "$UPDATE" = only ] && [ "$DOCKER" = yes ]; then
>      echo "Combining --docker and --update-only is not supported"

Pre-existing bug,but this error message should say
"--update-tools-only".

>      exit 1
>  fi

> @@ -343,7 +350,9 @@ fi
>
>  check_upload_permissions
>
> -update_coverity_tools
> +if [ "$UPDATE" != no ]; then
> +    update_coverity_tools
> +fi
>
>  TOOLBIN="$(cd "$COVERITY_TOOL_BASE" && echo $PWD/coverity_tool/cov-analysis-*/bin)"

Do we fail in a confusing or a clean way if you try --no-update-tools
and you didn't actually have a pre-existing cached copy of them?
I guess it doesn't matter much since this isn't intended to be
used by a lot of people.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 5/8] run-coverity-scan: add --no-update-tools option

[Paolo Bonzini]

On 27/04/20 14:46, Peter Maydell wrote:
>> -if [ "$UPDATE_ONLY" = yes ] && [ "$DOCKER" = yes ]; then
>> +if [ "$UPDATE" = only ] && [ "$DOCKER" = yes ]; then
>>      echo "Combining --docker and --update-only is not supported"
> Pre-existing bug,but this error message should say
> "--update-tools-only".
> 

I noticed, however by the end of the series it was easier to add the
support...

Paolo

[PATCH 6/8] run-coverity-scan: use --no-update-tools in docker run

[Paolo Bonzini]

Tools are already updated via the docker build.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/coverity-scan/run-coverity-scan | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/scripts/coverity-scan/run-coverity-scan b/scripts/coverity-scan/run-coverity-scan
index 9403429849..0c2c0e4087 100755
--- a/scripts/coverity-scan/run-coverity-scan
+++ b/scripts/coverity-scan/run-coverity-scan
@@ -318,17 +318,16 @@ if [ "$DOCKER" = yes ]; then
     fi
     echo "Archiving sources to be analyzed..."
     ./scripts/archive-source.sh "$SECRETDIR/qemu-sources.tgz"
+    ARGS="--no-update-tools"
     if [ "$DRYRUN" = yes ]; then
-        DRYRUNARG=--dry-run
+        ARGS="$ARGS --dry-run"
     fi
     echo "Running scanner..."
     # If we need to capture the output tarball, get the inner run to
     # save it to the secrets directory so we can copy it out before the
     # directory is cleaned up.
     if [ ! -z "$RESULTSTARBALL" ]; then
-        RTARGS="--results-tarball /work/cov-int.tar.xz"
-    else
-        RTARGS=""
+        ARGS="$ARGS --results-tarball /work/cov-int.tar.xz"
     fi
     # Arrange for this docker run to get access to the sources with -v.
     # We pass through all the configuration from the outer script to the inner.
@@ -336,8 +335,8 @@ if [ "$DOCKER" = yes ]; then
     tests/docker/docker.py run -it --env COVERITY_EMAIL --env COVERITY_BUILD_CMD \
            -v "$SECRETDIR:/work" coverity-scanner \
            ./run-coverity-scan --version "$VERSION" \
-           --description "$DESCRIPTION" $DRYRUNARG --tokenfile /work/token \
-           --srcdir /qemu --src-tarball /work/qemu-sources.tgz $RTARGS
+           --description "$DESCRIPTION" $ARGS --tokenfile /work/token \
+           --srcdir /qemu --src-tarball /work/qemu-sources.tgz
     if [ ! -z "$RESULTSTARBALL" ]; then
         echo "Copying results tarball to $RESULTSTARBALL..."
         cp "$SECRETDIR/cov-int.tar.xz" "$RESULTSTARBALL"
-- 
2.18.2

Re: [PATCH 6/8] run-coverity-scan: use --no-update-tools in docker run

[Peter Maydell]

On Wed, 22 Apr 2020 at 18:24, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Tools are already updated via the docker build.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 7/8] run-coverity-scan: download tools outside the container

[Paolo Bonzini]

This lets us look at coverity_tool.md5 across executions of run-coverity-scan
and skip the download.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/coverity-scan/coverity-scan.docker |  3 +-
 scripts/coverity-scan/run-coverity-scan    | 42 +++++++++++-----------
 2 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/scripts/coverity-scan/coverity-scan.docker b/scripts/coverity-scan/coverity-scan.docker
index 6f0460b66c..efcf63224d 100644
--- a/scripts/coverity-scan/coverity-scan.docker
+++ b/scripts/coverity-scan/coverity-scan.docker
@@ -127,5 +127,6 @@ RUN dnf install -y $PACKAGES
 RUN rpm -q $PACKAGES | sort > /packages.txt
 ENV PATH $PATH:/usr/libexec/python3-sphinx/
 ENV COVERITY_TOOL_BASE=/coverity-tools
+COPY coverity_tool.tgz coverity_tool.tgz
+RUN mkdir -p /coverity-tools/coverity_tool && cd /coverity-tools/coverity_tool && tar xf /coverity_tool.tgz
 COPY run-coverity-scan run-coverity-scan
-RUN ./run-coverity-scan --update-tools-only --tokenfile /work/token
diff --git a/scripts/coverity-scan/run-coverity-scan b/scripts/coverity-scan/run-coverity-scan
index 0c2c0e4087..49df8dcc06 100755
--- a/scripts/coverity-scan/run-coverity-scan
+++ b/scripts/coverity-scan/run-coverity-scan
@@ -114,15 +114,17 @@ update_coverity_tools () {
             echo "Downloaded tarball didn't match md5sum!"
             exit 1
         fi
-        # extract the new one, keeping it corralled in a 'coverity_tool' directory
-        echo "Unpacking coverity build tools..."
-        mkdir -p coverity_tool
-        cd coverity_tool
-        tar xf ../coverity_tool.tgz
-        cd ..
-        mv coverity_tool.md5.new coverity_tool.md5
-    fi
 
+        if [ "$DOCKER" != yes ]; then
+            # extract the new one, keeping it corralled in a 'coverity_tool' directory
+            echo "Unpacking coverity build tools..."
+            mkdir -p coverity_tool
+            cd coverity_tool
+            tar xf ../coverity_tool.tgz
+            cd ..
+            mv coverity_tool.md5.new coverity_tool.md5
+        fi
+    fi
     rm -f coverity_tool.md5.new
 }
 
@@ -289,6 +291,14 @@ if [ -z "$COVERITY_EMAIL" ]; then
     COVERITY_EMAIL="$(git config user.email)"
 fi
 
+# Otherwise, continue with the full build and upload process.
+
+check_upload_permissions
+
+if [ "$UPDATE" != no ]; then
+    update_coverity_tools
+fi
+
 # Run ourselves inside docker if that's what the user wants
 if [ "$DOCKER" = yes ]; then
     # Put the Coverity token into a temporary file that only
@@ -308,13 +318,13 @@ if [ "$DOCKER" = yes ]; then
     if [ "$UPDATE" != no ]; then
         # build docker container including the coverity-scan tools
         echo "Building docker container..."
-        # TODO: This re-downloads the tools every time, rather than
-        # caching and reusing the image produced with the downloaded tools.
+        # TODO: This re-unpacks the tools every time, rather than caching
+        # and reusing the image produced by the COPY of the .tgz file.
         # Not sure why.
         tests/docker/docker.py --engine ${DOCKER_ENGINE} build \
                        -t coverity-scanner -f scripts/coverity-scan/coverity-scan.docker \
-                       -v "$SECRETDIR:/work" \
-                       --extra-files scripts/coverity-scan/run-coverity-scan
+                       --extra-files scripts/coverity-scan/run-coverity-scan \
+                                     "$COVERITY_TOOL_BASE"/coverity_tool.tgz
     fi
     echo "Archiving sources to be analyzed..."
     ./scripts/archive-source.sh "$SECRETDIR/qemu-sources.tgz"
@@ -345,14 +355,6 @@ if [ "$DOCKER" = yes ]; then
     exit 0
 fi
 
-# Otherwise, continue with the full build and upload process.
-
-check_upload_permissions
-
-if [ "$UPDATE" != no ]; then
-    update_coverity_tools
-fi
-
 TOOLBIN="$(cd "$COVERITY_TOOL_BASE" && echo $PWD/coverity_tool/cov-analysis-*/bin)"
 
 if ! test -x "$TOOLBIN/cov-build"; then
-- 
2.18.2

Re: [PATCH 7/8] run-coverity-scan: download tools outside the container

[Peter Maydell]

On Wed, 22 Apr 2020 at 18:24, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> This lets us look at coverity_tool.md5 across executions of run-coverity-scan
> and skip the download.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 8/8] run-coverity-scan: support --update-tools-only --docker

[Paolo Bonzini]

Just build the container when run-coverity-scan is invoked with
--update-tools-only --docker.  This requires moving the "docker build"
logic into the update_coverity_tools function.

The only snag is that --update-tools-only --docker requires access to
the dockerfile.  For now just report an error for --src-tarball, and
"docker build" will fail if not in a source tree.  Another possibility
could be to host our container images on a public registry, and use
"FROM qemu:fedora" to make the Dockerfile small enough that it can be
included directly in the run-coverity-scan script.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/coverity-scan/run-coverity-scan | 39 +++++++++++++++----------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/scripts/coverity-scan/run-coverity-scan b/scripts/coverity-scan/run-coverity-scan
index 49df8dcc06..900ce9dd14 100755
--- a/scripts/coverity-scan/run-coverity-scan
+++ b/scripts/coverity-scan/run-coverity-scan
@@ -93,6 +93,18 @@ check_upload_permissions() {
 }
 
 
+build_docker_image() {
+    # build docker container including the coverity-scan tools
+    echo "Building docker container..."
+    # TODO: This re-unpacks the tools every time, rather than caching
+    # and reusing the image produced by the COPY of the .tgz file.
+    # Not sure why.
+    tests/docker/docker.py --engine ${DOCKER_ENGINE} build \
+                   -t coverity-scanner -f scripts/coverity-scan/coverity-scan.docker \
+                   --extra-files scripts/coverity-scan/run-coverity-scan \
+                                 "$COVERITY_TOOL_BASE"/coverity_tool.tgz
+}
+
 update_coverity_tools () {
     # Check for whether we need to download the Coverity tools
     # (either because we don't have a copy, or because it's out of date)
@@ -126,6 +138,11 @@ update_coverity_tools () {
         fi
     fi
     rm -f coverity_tool.md5.new
+    cd "$SRCDIR"
+
+    if [ "$DOCKER" = yes ]; then
+        build_docker_image
+    fi
 }
 
 
@@ -245,15 +262,16 @@ fi
 PROJNAME=QEMU
 TARBALL=cov-int.tar.xz
 
-if [ "$UPDATE" = only ] && [ "$DOCKER" = yes ]; then
-    echo "Combining --docker and --update-only is not supported"
-    exit 1
-fi
-
 if [ "$UPDATE" = only ]; then
     # Just do the tools update; we don't need to check whether
     # we are in a source tree or have upload rights for this,
     # so do it before some of the command line and source tree checks.
+
+    if [ "$DOCKER" = yes ] && [ ! -z "$SRCTARBALL" ]; then
+        echo --update-tools-only --docker is incompatible with --src-tarball.
+        exit 1
+    fi
+
     update_coverity_tools
     exit 0
 fi
@@ -315,17 +333,6 @@ if [ "$DOCKER" = yes ]; then
     echo "Created temporary directory $SECRETDIR"
     SECRET="$SECRETDIR/token"
     echo "$COVERITY_TOKEN" > "$SECRET"
-    if [ "$UPDATE" != no ]; then
-        # build docker container including the coverity-scan tools
-        echo "Building docker container..."
-        # TODO: This re-unpacks the tools every time, rather than caching
-        # and reusing the image produced by the COPY of the .tgz file.
-        # Not sure why.
-        tests/docker/docker.py --engine ${DOCKER_ENGINE} build \
-                       -t coverity-scanner -f scripts/coverity-scan/coverity-scan.docker \
-                       --extra-files scripts/coverity-scan/run-coverity-scan \
-                                     "$COVERITY_TOOL_BASE"/coverity_tool.tgz
-    fi
     echo "Archiving sources to be analyzed..."
     ./scripts/archive-source.sh "$SECRETDIR/qemu-sources.tgz"
     ARGS="--no-update-tools"
-- 
2.18.2

Re: [PATCH 8/8] run-coverity-scan: support --update-tools-only --docker

[Peter Maydell]

On Wed, 22 Apr 2020 at 18:26, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Just build the container when run-coverity-scan is invoked with
> --update-tools-only --docker.  This requires moving the "docker build"
> logic into the update_coverity_tools function.
>
> The only snag is that --update-tools-only --docker requires access to
> the dockerfile.  For now just report an error for --src-tarball, and
> "docker build" will fail if not in a source tree.  Another possibility
> could be to host our container images on a public registry, and use
> "FROM qemu:fedora" to make the Dockerfile small enough that it can be
> included directly in the run-coverity-scan script.

FWIW, one reason I didn't use 'FROM qemu:fedora' (or the local
equivalent) was because of your report that newer Fedora versions
broke the coverity tools.

>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM