Re: [Qemu-devel] [PATCH] Acceptance tests: host arch to target arch name mapping

[Cleber Rosa]

On 10/17/18 6:09 AM, Philippe Mathieu-Daudé wrote:
> Hi Cleber,
> 
> On 17/10/2018 01:22, Cleber Rosa wrote:
>> The host arch name is not always the target arch name, so it's
>> necessary to have a mapping.
>>
>> The configure scripts contains what is the authoritative and failproof
>> mapping, but, reusing it is not straightforward, so it's replicated in
>> the acceptance tests supporting code.
>>
>> Signed-off-by: Cleber Rosa <address@hidden>
>> ---
>>  configure                                 |  2 ++
>>  tests/acceptance/avocado_qemu/__init__.py | 23 +++++++++++++++++++++++
>>  2 files changed, 25 insertions(+)
>>
>> diff --git a/configure b/configure
>> index 8af2be959f..e029b756d4 100755
>> --- a/configure
>> +++ b/configure
>> @@ -6992,6 +6992,8 @@ TARGET_ARCH="$target_name"
>>  TARGET_BASE_ARCH=""
>>  TARGET_ABI_DIR=""
>>  
>> +# When updating target_name => TARGET_ARCH, please also update the
>> +# HOST_TARGET_ARCH mapping in tests/acceptance/avocado_qemu/__init__.py
>>  case "$target_name" in
>>    i386)
>>      mttcg="yes"
>> diff --git a/tests/acceptance/avocado_qemu/__init__.py 
>> b/tests/acceptance/avocado_qemu/__init__.py
>> index 1e54fd5932..d9bc4736ec 100644
>> --- a/tests/acceptance/avocado_qemu/__init__.py
>> +++ b/tests/acceptance/avocado_qemu/__init__.py
> 
> I'd put this in scripts/qemu.py
> 

I chose avocado_qemu/__init__.py because this mapping is currently of no
use to scripts/qemu.py.

But, I'm fine with moving it to scripts/qemu.py if most people agree.

>> @@ -19,6 +19,28 @@ sys.path.append(os.path.join(SRC_ROOT_DIR, 'scripts'))
>>  
>>  from qemu import QEMUMachine
>>  
>> +
>> +#: Mapping of host arch names to target arch names.  It's expected that the
>> +#: arch identification on the host, using os.uname()[4], would return the
>> +#: key (LHS).  The QEMU target name, and consequently the target binary, 
>> would
>> +#: be based on the name on the value (RHS).
>> +HOST_TARGET_ARCH = {
>> +    'armeb': 'arm',
>> +    'aarch64_be': 'aarch64',
> 
> Since you add this, I'd start directly with an exhaustive list:
> 
>        'aarch64_be': {'arch': 'aarch64', 'endian': 'big', 'wordsize':
> 64, 'abi' = 'arm'},
> 

Forgive my lack of QEMU culture, but the modus operandi I've been
following is to start with the simplest possible approach, as
justifiable by an existing requirement.

TBH, I lack knowledge about an use case for these other fields.

>> +    'microblazeel': 'microblaze',
>> +    'mipsel': 'mips',
>> +    'mipsn32el' : 'mips64',
>> +    'mips64el': 'mips64',
>> +    'or1k': 'openrisc',
>> +    'ppc64le': 'ppc64',
>> +    'ppc64abi32': 'ppc64',
>> +    'riscv64': 'riscv',
>> +    'sh4eb': 'sh4',
>> +    'sparc32plus': 'sparc64',
>> +    'xtensaeb': 'xtensa'
>> +    }
> 
> Then a function such:
> 
> def target_normalize(key, arch):
>     return table[arch][key]
> 
>> +
>> +
>>  def is_readable_executable_file(path):
>>      return os.path.isfile(path) and os.access(path, os.R_OK | os.X_OK)
>>  
>> @@ -29,6 +51,7 @@ def pick_default_qemu_bin():
>>      directory or in the source tree root directory.
>>      """
>>      arch = os.uname()[4]
>> +    arch = HOST_TARGET_ARCH.get(arch, arch)
> 
>        arch = target_normalize('arch', os.uname()[4])
> 
>>      qemu_bin_relative_path = os.path.join("%s-softmmu" % arch,
>>                                            "qemu-system-%s" % arch)
>>      if is_readable_executable_file(qemu_bin_relative_path):
>>
> 
> What do you think?
> 

Python dictionaries are so rich that can and are encouraged to be used
in place of such functions and even "switch/case" constructs.  I would
add such a function only if there's extra logic no present in a dict.

Since we're more or less building a coding style here, it'd be useful to
hear what other people think.

Thanks for the review!
- Cleber.

> Thanks,
> 
> Phil.
> 

-- 
Cleber Rosa
[ Sr Software Engineer - Virtualization Team - Red Hat ]
[ Avocado Test Framework - avocado-framework.github.io ]
[  7ABB 96EB 8B46 B94D 5E0F  E9BB 657E 8D33 A5F2 09F3  ]