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 <cr...@redhat.com>
>> ---
>> 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 ]
