Re: [Qemu-devel] [PATCH V2] linux-user: fix QEMU_STRACE=1 segfault

[Peter Maydell]

On 21 November 2011 10:40, Alexander Graf ag...@suse.de wrote:
 diff --git a/linux-user/strace.c b/linux-user/strace.c
 index 90027a1..e30b005 100644
 --- a/linux-user/strace.c
 +++ b/linux-user/strace.c
 @@ -1515,14 +1515,19 @@ void
  print_syscall_ret(int num, abi_long ret)
  {
     int i;
 +    char *errstr = NULL;

     for(i=0;insyscalls;i++)
         if( scnames[i].nr == num ) {
             if( scnames[i].result != NULL ) {
                 scnames[i].result(scnames[i],ret);
             } else {
 -                if( ret  0 ) {
 -                    gemu_log( = -1 errno= TARGET_ABI_FMT_ld  (%s)\n, 
 -ret, target_strerror(-ret));
 +                if (ret  0) {
 +                    errstr = target_strerror(-ret);
 +                }
 +                if (errstr) {
 +                    gemu_log( = -1 errno= TARGET_ABI_FMT_ld  (%s)\n,
 +                             -ret, errstr);

Should this really be printing -1 all the time when ret isn't -1 ?

                 } else {
                     gemu_log( =  TARGET_ABI_FMT_ld \n, ret);
                 }

print_syscall_ret_addr() also needs to handle NULL
returns from target_strerror().

 diff --git a/linux-user/syscall.c b/linux-user/syscall.c
 index f227097..c59d00e 100644
 --- a/linux-user/syscall.c
 +++ b/linux-user/syscall.c
 @@ -731,6 +731,9 @@ static inline int is_error(abi_long ret)

  char *target_strerror(int err)
  {
 +    if ((err = ERRNO_TABLE_SIZE)  (err = 0)) {
 +        return NULL;
 +    }
     return strerror(target_to_host_errno(err));

shouldn't that be ((err = ERRNO_TABLE_SIZE) || (err  0)) ?

-- PMM

Re: [Qemu-devel] [PATCH V2] linux-user: fix QEMU_STRACE=1 segfault

[Alexander Graf]

On 21.11.2011, at 11:47, Peter Maydell wrote:

 On 21 November 2011 10:40, Alexander Graf ag...@suse.de wrote:
 diff --git a/linux-user/strace.c b/linux-user/strace.c
 index 90027a1..e30b005 100644
 --- a/linux-user/strace.c
 +++ b/linux-user/strace.c
 @@ -1515,14 +1515,19 @@ void
  print_syscall_ret(int num, abi_long ret)
  {
 int i;
 +char *errstr = NULL;
 
 for(i=0;insyscalls;i++)
 if( scnames[i].nr == num ) {
 if( scnames[i].result != NULL ) {
 scnames[i].result(scnames[i],ret);
 } else {
 -if( ret  0 ) {
 -gemu_log( = -1 errno= TARGET_ABI_FMT_ld  (%s)\n, 
 -ret, target_strerror(-ret));
 +if (ret  0) {
 +errstr = target_strerror(-ret);
 +}
 +if (errstr) {
 +gemu_log( = -1 errno= TARGET_ABI_FMT_ld  (%s)\n,
 + -ret, errstr);
 
 Should this really be printing -1 all the time when ret isn't -1 ?

In linux-user, the syscall emulation functions return -errno which later gets 
translated to -1 with errno=-errno. That's why the code looks so weird. So yes, 
it's correct.

 
 } else {
 gemu_log( =  TARGET_ABI_FMT_ld \n, ret);
 }
 
 print_syscall_ret_addr() also needs to handle NULL
 returns from target_strerror().

I don't think they can ever happen, since we're sure to use valid errnos there. 
But I can add a check.

 
 diff --git a/linux-user/syscall.c b/linux-user/syscall.c
 index f227097..c59d00e 100644
 --- a/linux-user/syscall.c
 +++ b/linux-user/syscall.c
 @@ -731,6 +731,9 @@ static inline int is_error(abi_long ret)
 
  char *target_strerror(int err)
  {
 +if ((err = ERRNO_TABLE_SIZE)  (err = 0)) {
 +return NULL;
 +}
 return strerror(target_to_host_errno(err));
 
 shouldn't that be ((err = ERRNO_TABLE_SIZE) || (err  0)) ?

Yes.


Alex

Re: [Qemu-devel] [PATCH V2] linux-user: fix QEMU_STRACE=1 segfault

[Peter Maydell]

On 21 November 2011 10:52, Alexander Graf ag...@suse.de wrote:
 On 21.11.2011, at 11:47, Peter Maydell wrote:
 Should this really be printing -1 all the time when ret isn't -1 ?

 In linux-user, the syscall emulation functions return -errno which later
 gets translated to -1 with errno=-errno. That's why the code looks so
 weird. So yes, it's correct.

Yes, but the syscall ABI (which is what we're implementing) returns
-errno for errors; the errno variable is a purely userspace construct.
I'd expected that the strace layer would be tracing exactly what
the syscall ABI was.

However it looks like standard userspace strace(1) also does this
translation to -1 errno=thing, so it makes sense for us to do that too.

-- PMM