[Ltrace-devel] [PATCH 00/11] libdl support in ltrace - please test!

[Petr Machata]

09.11.2010 00:47, Joe Damato wrote:
> List,
>
> I've broken apart my lib-dl changes into a series of smaller patches to make
> reading and understanding them a bit less painful. Not sure I'll squash these
> prior to pulling code into the official repository, but for now I think keeping
> them separate so people can read them is probably preferred.
>
> These should apply to my master at http://github.com/ice799/ltrace.
>
> I added a simple unit test for these changes that should run automaticall when
> you run "make check."
>
> I have tested these on x86_64 and x86 Ubuntu VMs.
>
> I have no idea if this code works on other architectures and would appreciate
> testers.

Hey, thanks for the patchset.  I was able to look through today and have 
a couple nits to pick.

In the umovebytes function, your error handling looks strange.  First, 
errno is only valid if the return value indicates that it might be. 
Most likely you need this:

  a.a = ptrace(PTRACE_PEEKTEXT, proc->pid, addr + offset, 0);
-if (errno) {
+if (a.a == -1 && errno) {
  	if (started && (errno == EPERM || errno == EIO))

It's a good thing that you introduced some error checking to ptrace, 
it's largely ignored in the rest of code.  But I don't understand the 
logic behind ignoring some error codes and respecting others:

   if (started && (errno == EPERM || errno == EIO))
     return bytes_read;
   if (addr != 0 && errno != EIO && errno != ESRCH)
     return -1;

EPERM, per the man, means that we can't trace the process.  Well, at 
this point we know we should be able to trace it, and this error code 
turning up is a sign of trouble.  I'd leave just EIO in that branch, for 
hitting inaccessible area of memory.

The second branch I don't understand at all.  Why do we tolerate ESRCH? 
  That's similarly bad as EPERM in my opinion.  And why do we just keep 
on reading on NULL, errors or no errors?

In short, it seems to me that what we need is this:
   if (started && errno == EIO)
     return bytes_read;
   else
     return -1;
Perhaps with the exception of EBUSY, which we might want to retry a 
couple times before bailing out.

Then there's the business with one global libdl_hooked.  Why global? 
Shouldn't this be inside struct Process?  This has to break in the face 
of follow-forks and the like.

Finally there was one case of refactoring and one bug fix that I fixed 
on my branch "test-fixes":
https://github.com/pmachata/ltrace/commits/test-fixes

Overall it's nice to have this functionality in ltrace, thanks for the work!

PM