Re: [Qemu-devel] [PATCH 02/22] configure: early test for supported targets

["Daniel P. Berrange" <berrange@redhat.com>]

On Mon, Jul 03, 2017 at 06:34:33PM +0200, Paolo Bonzini wrote:
> Check for unsupported targets in target_list, and print an
> error early in the configuration process.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  configure | 65 ++++++++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 43 insertions(+), 22 deletions(-)
> 
> diff --git a/configure b/configure
> index 0f14e79..6d1a6a9 100755
> --- a/configure
> +++ b/configure
> @@ -40,14 +40,18 @@ printf " '%s'" "$0" "$@" >> config.log
>  echo >> config.log
>  echo "#" >> config.log
>  
> -error_exit() {
> -    echo
> +print_error() {
> +    (echo
>      echo "ERROR: $1"
>      while test -n "$2"; do
>          echo "       $2"
>          shift
>      done
> -    echo
> +    echo) >&2
> +}
> +
> +error_exit() {
> +    print_error "$@"
>      exit 1
>  }
>  
> @@ -207,6 +211,25 @@ supported_xen_target() {
>      return 1
>  }
>  
> +supported_target() {
> +    case "$1" in
> +        ${target_name}-softmmu) ;;

Nit-pick, I'd prefer the ';;' on a separate line, as currently it at
first looks like the -softmmu line will fallthrough to the -linux-user
block below. Took a while before I noticed the trailing ;;

Or alternatively add a blank line between cases

> +        ${target_name}-linux-user)
> +            if test "$linux" != "yes"; then
> +                print_error "Target '$target' is only available on a Linux host"
> +                return 1
> +            fi
> +            ;;
> +        ${target_name}-bsd-user)
> +            if test "$bsd" != "yes"; then
> +                print_error "Target '$target' is only available on a BSD host"
> +                return 1
> +            fi
> +            ;;

Add a wildcard match as a catch-all to avoid falling through to the
success codepath on unknown items ?

> +    esac
> +    return 0
> +}
> +
>  # default parameters
>  source_path=$(dirname "$0")
>  cpu=""
> @@ -1734,23 +1757,27 @@ if test "$solaris" = "yes" ; then
>  fi
>  
>  if test -z "${target_list+xxx}" ; then
> -    target_list="$default_target_list"
> +    for target in $default_target_list; do
> +        supported_target $target 2>/dev/null && \
> +            target_list="$target_list $target"
> +    done

I'm not sure what the benefit of this change is ?  It is calling
supported_target but throwing away anything it prints. So if there
was a mistake in the default target list setup, we'd never see it,
just silently discard it.

> +    target_list="${target_list# }"
>  else
>      target_list=$(echo "$target_list" | sed -e 's/,/ /g')
> +    for target in $target_list; do
> +        # Check that we recognised the target name; this allows a more
> +        # friendly error message than if we let it fall through.
> +        case " $default_target_list " in
> +            *" $target "*)
> +                ;;
> +            *)
> +                error_exit "Unknown target name '$target'"
> +                ;;
> +        esac
> +        supported_target $target || exit 1
> +    done
>  fi
>  
> -# Check that we recognised the target name; this allows a more
> -# friendly error message than if we let it fall through.
> -for target in $target_list; do
> -    case " $default_target_list " in
> -        *" $target "*)
> -            ;;
> -        *)
> -            error_exit "Unknown target name '$target'"
> -            ;;
> -    esac
> -done
> -

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|