| Message ID | 20221223100107.17303-1-deller@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | linux-user: Improve strace output of personality() and sysinfo() | expand |
On 23/12/22 11:01, Helge Deller wrote: > Make the strace look nicer for those two syscalls. > > Signed-off-by: Helge Deller <deller@gmx.de> > --- > linux-user/strace.list | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/linux-user/strace.list b/linux-user/strace.list > index f9254725a1..909298099e 100644 > --- a/linux-user/strace.list > +++ b/linux-user/strace.list > @@ -1043,7 +1043,7 @@ > { TARGET_NR_perfctr, "perfctr" , NULL, NULL, NULL }, > #endif > #ifdef TARGET_NR_personality > -{ TARGET_NR_personality, "personality" , NULL, NULL, NULL }, > +{ TARGET_NR_personality, "personality" , "%s(%p)", NULL, print_syscall_ret_addr }, Shouldn't this be: { TARGET_NR_personality, "personality" , "%s(%u)", NULL, NULL }, ? > #endif > #ifdef TARGET_NR_pipe > { TARGET_NR_pipe, "pipe" , NULL, NULL, NULL }, > @@ -1502,7 +1502,7 @@ > { TARGET_NR_sysfs, "sysfs" , NULL, NULL, NULL }, > #endif > #ifdef TARGET_NR_sysinfo > -{ TARGET_NR_sysinfo, "sysinfo" , NULL, NULL, NULL }, > +{ TARGET_NR_sysinfo, "sysinfo" , "%s(%p)", NULL, NULL }, > #endif > #ifdef TARGET_NR_sys_kexec_load > { TARGET_NR_sys_kexec_load, "sys_kexec_load" , NULL, NULL, NULL }, > -- > 2.38.1 > >
On 12/23/22 11:50, Philippe Mathieu-Daudé wrote: > On 23/12/22 11:01, Helge Deller wrote: >> Make the strace look nicer for those two syscalls. >> >> Signed-off-by: Helge Deller <deller@gmx.de> >> --- >> linux-user/strace.list | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/linux-user/strace.list b/linux-user/strace.list >> index f9254725a1..909298099e 100644 >> --- a/linux-user/strace.list >> +++ b/linux-user/strace.list >> @@ -1043,7 +1043,7 @@ >> { TARGET_NR_perfctr, "perfctr" , NULL, NULL, NULL }, >> #endif >> #ifdef TARGET_NR_personality >> -{ TARGET_NR_personality, "personality" , NULL, NULL, NULL }, >> +{ TARGET_NR_personality, "personality" , "%s(%p)", NULL, print_syscall_ret_addr }, > > Shouldn't this be: > > { TARGET_NR_personality, "personality" , "%s(%u)", NULL, NULL }, Basically yes, but... it's a bitmap, so printing it as hex value (similiar to a pointer) is easier to read/analyze. Helge > > ? > >> #endif >> #ifdef TARGET_NR_pipe >> { TARGET_NR_pipe, "pipe" , NULL, NULL, NULL }, >> @@ -1502,7 +1502,7 @@ >> { TARGET_NR_sysfs, "sysfs" , NULL, NULL, NULL }, >> #endif >> #ifdef TARGET_NR_sysinfo >> -{ TARGET_NR_sysinfo, "sysinfo" , NULL, NULL, NULL }, >> +{ TARGET_NR_sysinfo, "sysinfo" , "%s(%p)", NULL, NULL }, >> #endif >> #ifdef TARGET_NR_sys_kexec_load >> { TARGET_NR_sys_kexec_load, "sys_kexec_load" , NULL, NULL, NULL }, >> -- >> 2.38.1 >> >> >
On 23/12/22 11:53, Helge Deller wrote: > On 12/23/22 11:50, Philippe Mathieu-Daudé wrote: >> On 23/12/22 11:01, Helge Deller wrote: >>> Make the strace look nicer for those two syscalls. >>> >>> Signed-off-by: Helge Deller <deller@gmx.de> >>> --- >>> linux-user/strace.list | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/linux-user/strace.list b/linux-user/strace.list >>> index f9254725a1..909298099e 100644 >>> --- a/linux-user/strace.list >>> +++ b/linux-user/strace.list >>> @@ -1043,7 +1043,7 @@ >>> { TARGET_NR_perfctr, "perfctr" , NULL, NULL, NULL }, >>> #endif >>> #ifdef TARGET_NR_personality >>> -{ TARGET_NR_personality, "personality" , NULL, NULL, NULL }, >>> +{ TARGET_NR_personality, "personality" , "%s(%p)", NULL, >>> print_syscall_ret_addr }, >> >> Shouldn't this be: >> >> { TARGET_NR_personality, "personality" , "%s(%u)", NULL, NULL }, > > Basically yes, but... > it's a bitmap, so printing it as hex value (similiar to a pointer) > is easier to read/analyze. Oh, good point. Then "%s(0x"TARGET_ABI_FMT_lx")" is self-explicit. Also for clarity: #define print_syscall_ret_persona print_syscall_ret_addr So what do you think of: { TARGET_NR_personality, "personality" , "%s(0x"TARGET_ABI_FMT_lx")", NULL, print_syscall_ret_persona }, ?
On 12/23/22 12:01, Philippe Mathieu-Daudé wrote: > On 23/12/22 11:53, Helge Deller wrote: >> On 12/23/22 11:50, Philippe Mathieu-Daudé wrote: >>> On 23/12/22 11:01, Helge Deller wrote: >>>> Make the strace look nicer for those two syscalls. >>>> >>>> Signed-off-by: Helge Deller <deller@gmx.de> >>>> --- >>>> linux-user/strace.list | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/linux-user/strace.list b/linux-user/strace.list >>>> index f9254725a1..909298099e 100644 >>>> --- a/linux-user/strace.list >>>> +++ b/linux-user/strace.list >>>> @@ -1043,7 +1043,7 @@ >>>> { TARGET_NR_perfctr, "perfctr" , NULL, NULL, NULL }, >>>> #endif >>>> #ifdef TARGET_NR_personality >>>> -{ TARGET_NR_personality, "personality" , NULL, NULL, NULL }, >>>> +{ TARGET_NR_personality, "personality" , "%s(%p)", NULL, print_syscall_ret_addr }, >>> >>> Shouldn't this be: >>> >>> { TARGET_NR_personality, "personality" , "%s(%u)", NULL, NULL }, >> >> Basically yes, but... >> it's a bitmap, so printing it as hex value (similiar to a pointer) >> is easier to read/analyze. > > Oh, good point. Then "%s(0x"TARGET_ABI_FMT_lx")" is self-explicit. Hmm ... I don't see that as any benefit for the user and the output is the same. > Also for clarity: > > #define print_syscall_ret_persona print_syscall_ret_addr > > So what do you think of: > > { TARGET_NR_personality, "personality" , "%s(0x"TARGET_ABI_FMT_lx")", > NULL, print_syscall_ret_persona }, This change does make sense if someone fully implements the strace of personality() including showing the flags as string. Until then it's IMHO just one more function which uses space and gains nothing. Helge
Le 23/12/2022 à 13:27, Helge Deller a écrit : > On 12/23/22 12:01, Philippe Mathieu-Daudé wrote: >> On 23/12/22 11:53, Helge Deller wrote: >>> On 12/23/22 11:50, Philippe Mathieu-Daudé wrote: >>>> On 23/12/22 11:01, Helge Deller wrote: >>>>> Make the strace look nicer for those two syscalls. >>>>> >>>>> Signed-off-by: Helge Deller <deller@gmx.de> >>>>> --- >>>>> linux-user/strace.list | 4 ++-- >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/linux-user/strace.list b/linux-user/strace.list >>>>> index f9254725a1..909298099e 100644 >>>>> --- a/linux-user/strace.list >>>>> +++ b/linux-user/strace.list >>>>> @@ -1043,7 +1043,7 @@ >>>>> { TARGET_NR_perfctr, "perfctr" , NULL, NULL, NULL }, >>>>> #endif >>>>> #ifdef TARGET_NR_personality >>>>> -{ TARGET_NR_personality, "personality" , NULL, NULL, NULL }, >>>>> +{ TARGET_NR_personality, "personality" , "%s(%p)", NULL, print_syscall_ret_addr }, >>>> >>>> Shouldn't this be: >>>> >>>> { TARGET_NR_personality, "personality" , "%s(%u)", NULL, NULL }, >>> >>> Basically yes, but... >>> it's a bitmap, so printing it as hex value (similiar to a pointer) >>> is easier to read/analyze. >> >> Oh, good point. Then "%s(0x"TARGET_ABI_FMT_lx")" is self-explicit. > > Hmm ... I don't see that as any benefit for the user and the output is the same. > I agree with Philippe for this part, it's not a pointer, don't use %p. Thanks, Laurent
diff --git a/linux-user/strace.list b/linux-user/strace.list index f9254725a1..909298099e 100644 --- a/linux-user/strace.list +++ b/linux-user/strace.list @@ -1043,7 +1043,7 @@ { TARGET_NR_perfctr, "perfctr" , NULL, NULL, NULL }, #endif #ifdef TARGET_NR_personality -{ TARGET_NR_personality, "personality" , NULL, NULL, NULL }, +{ TARGET_NR_personality, "personality" , "%s(%p)", NULL, print_syscall_ret_addr }, #endif #ifdef TARGET_NR_pipe { TARGET_NR_pipe, "pipe" , NULL, NULL, NULL }, @@ -1502,7 +1502,7 @@ { TARGET_NR_sysfs, "sysfs" , NULL, NULL, NULL }, #endif #ifdef TARGET_NR_sysinfo -{ TARGET_NR_sysinfo, "sysinfo" , NULL, NULL, NULL }, +{ TARGET_NR_sysinfo, "sysinfo" , "%s(%p)", NULL, NULL }, #endif #ifdef TARGET_NR_sys_kexec_load { TARGET_NR_sys_kexec_load, "sys_kexec_load" , NULL, NULL, NULL },
Make the strace look nicer for those two syscalls. Signed-off-by: Helge Deller <deller@gmx.de> --- linux-user/strace.list | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.38.1