On Fri, Feb 13, 2009 at 11:37:07AM +0100, Denys Vlasenko wrote:
[...]
> Please review.

Sorry for the long delay.
The change looks good indeed.  See my comments below, though.

> +     if (do_ptrace(PTRACE_SET_SYSCALL, tcp, NULL, new, "SET_SYSCALL") != 0)

The "SET_SYSCALL" is deducable from PTRACE_SET_SYSCALL number, isn't it?
Could we avoid this redundancy?

>  long
> -do_ptrace(int request, struct tcb *tcp, void *addr, void *data)
> +do_ptrace(int request, struct tcb *tcp, void *addr, long data, const char 
> *msg)
>  {
> +     int err;
>       long l;
>  
>       errno = 0;
>       l = ptrace(request, tcp->pid, addr, data);
> -     /* Non-ESRCH errors might be our invalid reg/mem accesses,
> -      * we do not record them. */
> -     if (errno == ESRCH)
> -             tcp->ptrace_errno = ESRCH;
> +     err = errno;
> +     if (err) {
> +             tcp->ptrace_errno = err;
> +             if (err != ESRCH) {
> +                     fprintf(stderr, "strace: ptrace(PTRACE_%s,%u,%p,%lu): 
> %s\n",
> +                             msg, (int) tcp->pid, addr, data, strerror(err));
> +             }
> +             return -1;
> +     }
> +     return l;
> +}

fprintf() may clobber errno, which is tested after do_ptrace() return
several times in the code, so we have to either reset errno back to the
value set by ptrace() call, or change *all* do_ptrace() callers to check
its return value instead of errno.

> +static long
> +do_ptrace_peekdata(struct tcb *tcp, void *addr, int started)
> +{
> +     int err;
> +     long l;
> +
> +     errno = 0;
> +     l = ptrace(PTRACE_PEEKDATA, tcp->pid, addr, 0);
> +     err = errno;
> +     if (err) {
> +             if (started && (err == EPERM || err == EIO)) {
> +                     /* Ran into 'end of memory' - not an error */
> +                     return 0;
> +             }
> +             /* If error happens at first call, we have a bogus address. */
> +             if (addr != NULL && err != EIO) {
> +                     if (errno != ESRCH) {
> +                             fprintf(stderr, "strace: 
> ptrace(PTRACE_PEEKDATA,%u,%p,0): %s\n",
> +                                     (int) tcp->pid, addr, strerror(err));
> +                             perror("ptrace: umoven");
> +                     }
> +                     tcp->ptrace_errno = errno;
> +                     return -1;
> +             }
> +     }
>       return l;
>  }

Previous comment about errno is applicable to this function as well.


-- 
ldv

Attachment: pgpyxnAaCkW76.pgp
Description: PGP signature

------------------------------------------------------------------------------
Open Source Business Conference (OSBC), March 24-25, 2009, San Francisco, CA
-OSBC tackles the biggest issue in open source: Open Sourcing the Enterprise
-Strategies to boost innovation and cut costs with open source participation
-Receive a $600 discount off the registration fee with the source code: SFAD
http://p.sf.net/sfu/XcvMzF8H
_______________________________________________
Strace-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/strace-devel

Reply via email to