Re: [Qemu-devel] [PATCH for-2.9] Fix check for target OS support

[Stefan Weil]

Am 28.03.2017 um 09:10 schrieb Peter Maydell:
> On 27 March 2017 at 21:11, Stefan Weil <address@hidden> wrote:
>> This check had several problems which are fixed here:
>>
>> * Calling "configure --help" was no longer possible on Cygwin.
>>   Fix  this by introducing a third state for supported_os and
>>   by moving the error handling code.
>>   Move the error handling code for supported_cpu, too.
>>
>> * Fix the error text. Use target cpu and target os because the
>>   build cpu and build host don't matter here.
> I think it is important that we print the deprecation
> message last, after all the other configure output.
> Otherwise it will scroll off the screen and most people
> will miss it. That's why I put it where it is.

Ok, then only the error handling for unsupported target os has
to be moved (to support ./configure --help).

>> index d1ce33bc79..902bd20070 100755
>> --- a/configure
>> +++ b/configure
>> @@ -322,7 +322,7 @@ jemalloc="no"
>>  replication="yes"
>>
>>  supported_cpu="no"
>> -supported_os="no"
>> +supported_os="deprecated"
> I don't see any need for anything other than 'yes' or 'no' ?

yes = file
no = raise error
deprecated = raise warning

Needed for delayed error handling, see below.

>>  # parse CC options first
>>  for opt do
>> @@ -440,7 +440,13 @@ int main(void) { return 0; }
>>  EOF
>>  }
>>
>> -if check_define __linux__ ; then
>> +if test "$cross_prefix" = "i686_64-w64-mingw32-" ; then
>> +  targetos='MINGW32'
>> +elif test "$cross_prefix" = "x86_64-w64-mingw32-" ; then
>> +  targetos='MINGW32'
>> +elif test -n "$cross_prefix" ; then
>> +  targetos="$cross_prefix"
> This doesn't look right. We already have too many
> cases where we determine the target OS by something
> other than looking at check_defines; one thing
> I'd like to do when we've got rid of unsupported
> OSes is to make sure that we determine the target
> OS by check_define and only by check_define.

That part of my patch is not needed. I added it because I
did not realize that "check_define" already does the correct
thing and because a wrong cross prefix raises a misleading
error message.

>> +elif check_define __linux__ ; then
>>    targetos="Linux"
>>  elif check_define _WIN32 ; then
>>    targetos='MINGW32'
>> @@ -694,7 +700,7 @@ Linux)
>>    supported_os="yes"
>>  ;;
>>  *)
>> -  error_exit "Unsupported host OS $targetos"
>> +  supported_os="no"
> This was deliberately an error_exit because anything
> going down that path got the Linux defines/includes by
> accident and we thought that would not work on
> anything else. What was using it? If we need an extra
> entry in the case statement we can add one.

It remains an error (that's why I introduced a third state for
supported_os), but must be handled later, after processing
a potential --help option.

>>      echo "CONFIG_QGA_NTDDDISK=y" >> $config_host_mak
>>    fi
>>    if test "$guest_agent_msi" = "yes"; then
>> -    echo "QEMU_GA_MSI_ENABLED=yes" >> $config_host_mak
>> +    echo "QEMU_GA_MSI_ENABLED=yes" >> $config_host_mak
>>      echo "QEMU_GA_MSI_MINGW_DLL_PATH=${QEMU_GA_MSI_MINGW_DLL_PATH}" >> 
>> $config_host_mak
>>      echo "QEMU_GA_MSI_WITH_VSS=${QEMU_GA_MSI_WITH_VSS}" >> $config_host_mak
>>      echo "QEMU_GA_MSI_ARCH=${QEMU_GA_MSI_ARCH}" >> $config_host_mak
> Stray unintended change?

Whitespace at line ending removed by my editor.

Stefan