Re: [PATCH v2] linux-user: Improve strace output of pread64() and pwrite64()

[Helge Deller <deller@gmx.de>]

On 1/31/23 12:04, Laurent Vivier wrote:
> Le 30/01/2023 à 23:11, Helge Deller a écrit :
>> On 1/30/23 10:26, Laurent Vivier wrote:
>>> Le 27/01/2023 à 21:58, Helge Deller a écrit :
>>>> Make the strace look nicer for those two syscalls.
>>>>
>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>> ---
>>>> v2: Use regpairs_aligned() and target_offset64(), noticed by Laurent Vivier
>>>>
>>>> diff --git a/linux-user/strace.c b/linux-user/strace.c
>>>> index 82dc1a1e20..379536f5c9 100644
>>>> --- a/linux-user/strace.c
>>>> +++ b/linux-user/strace.c
>>>> @@ -3824,6 +3824,25 @@ print_rlimit64(abi_ulong rlim_addr, int last)
>>>>       }
>>>>   }
>>>>
>>>> +#if defined(TARGET_NR_pread64) || defined(TARGET_NR_pwrite64)
>>>> +static void
>>>> +print_preadwrite64(CPUArchState *cpu_env, const struct syscallname *name,
>>>> +           abi_long arg0, abi_long arg1, abi_long arg2,
>>>> +           abi_long arg3, abi_long arg4, abi_long arg5)
>>>> +{
>>>> +    if (regpairs_aligned(cpu_env, TARGET_NR_pread64)) {
>>>> +        arg3 = arg4;
>>>> +        arg4 = arg5;
>>>> +    }
>>>> +    print_syscall_prologue(name);
>>>> +    print_raw_param("%d", arg0, 0);
>>>> +    print_pointer(arg1, 0);
>>>> +    print_raw_param("%d", arg2, 0);
>>>> +    qemu_log("%lld", (long long)target_offset64(arg3, arg4));
>>>
>>> better to use:
>>>
>>> print_raw_param("%" PRIu64, target_offset64(arg3, arg4), 1);
>>
>> I thought of that as well, but that won't work, as print_raw_param()
>> takes an "abi_long" value, which is just a 32-bit value on 32-bit targets.
>> See print_rlimit64(), it's used there with qemu_log() as well.
>
> Yes, you're right.
>
> But even with qemu_log() I would prefer you use "%"PRIu64 rather than %lld.
> Or better define a print_raw_param64() (or similar) and update print_fallocate(), print_truncate64() and print_ftruncate64().

As adding such define is a unrelated change to this patch, I'd propose that I send a follow-up
patch on top of this one which adds print_raw_param64() (or similar) and replaces all
usages of qemu_log() with 64-bit values and use "%"PRIu64 then.
Ok?
If yes, should that define include the last 0/1 parameter to print the "," ?
I think so, because then it's consistent with print_raw_param().

Helge