Re: [PATCH] configure: Add 'mkdir build' check

[Peter Maydell]

On Sun, 5 Feb 2023 at 07:44, Dinah Baum <dinahbaum123@gmail.com> wrote:
>
> QEMU configure script goes into an infinite error printing loop
> when in read only directory due to 'build' dir never being created.
>
> Checking if 'mkdir dir' succeeds and if the directory is
> writeable prevents this error.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/321
>
> Signed-off-by: Dinah Baum <dinahbaum123@gmail.com>

Hi; thanks for sending this patch.

> ---
>  configure | 37 ++++++++++++++++++++++++++++++-------
>  1 file changed, 30 insertions(+), 7 deletions(-)
>
> diff --git a/configure b/configure
> index 64960c6000..fe9028991f 100755
> --- a/configure
> +++ b/configure
> @@ -32,9 +32,11 @@ then
>      fi
>
>      mkdir build
> -    touch $MARKER
> +    if [ -d build ] && [ -w build ]
> +    then
> +        touch $MARKER

It would be more straightforward to check whether
the 'mkdir' and 'touch' commands succeed, I think.

>
> -    cat > GNUmakefile <<'EOF'
> +        cat > GNUmakefile <<'EOF'
>  # This file is auto-generated by configure to support in-source tree
>  # 'make' command invocation
>
> @@ -56,8 +58,15 @@ force: ;
>  GNUmakefile: ;
>
>  EOF
> -    cd build
> -    exec "$source_path/configure" "$@"
> +        cd build
> +        exec "$source_path/configure" "$@"
> +    elif ! [ -d build ]
> +    then
> +        echo "ERROR: Unable to create ./build dir, try using a 
> ../qemu/configure build"
> +    elif ! [ -w build ]
> +    then
> +        echo "ERROR: ./build dir not writeable, try using a 
> ../qemu/configure build"
> +    fi

If these are errors, we should exit immediately, not
continue further trying to run code.

>  fi
>
>  # Temporary directory used for files created while
> @@ -181,9 +190,12 @@ compile_prog() {
>
>  # symbolically link $1 to $2.  Portable version of "ln -sf".
>  symlink() {
> -  rm -rf "$2"
> -  mkdir -p "$(dirname "$2")"
> -  ln -s "$1" "$2"
> +  if [ -d $source_path/build ] && [ -w $source_path/build ]
> +  then
> +      rm -rf "$2"
> +      mkdir -p "$(dirname "$2")"
> +      ln -s "$1" "$2"
> +  fi

The symlink function is a utility one used in various
places in the code. It may be used for other directories
than $source_path/build. If we need to better handle
errors here then we should do that by checking the
exit status of the commands (and probably passing the
return status back up for the caller to look at).

But there's a lot of code in configure that assumes it
can write to the destination directory elsewhere too,
so why change this function specifically ?

>  }
>
>  # check whether a command is available to this shell (may be either an
> @@ -2287,7 +2299,18 @@ fi
>  #######################################
>  # generate config-host.mak
>
> +if ! [ -d $source_path/build ] || ! [ -w $source_path/build ]

You can't assume that the build dir is always $source_path/build
-- that's just the default if the user ran configure from
the source directory.

> +then
> +    echo "ERROR: ./build dir unusable, exiting"
> +    # cleanup
> +    rm -f config.log
> +    rm -f Makefile.prereqs
> +    rm -r "$TMPDIR1"
> +    exit 1

Most of these haven't been created at this point, so don't
need to be deleted. (If you do the error-exit earlier,
as I suggest, then this is clearer.)

> +fi
> +
>  if ! (GIT="$git" "$source_path/scripts/git-submodule.sh" 
> "$git_submodules_action" "$git_submodules"); then
> +    echo "BAD"
>      exit 1
>  fi

thanks
-- PMM