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

[Joe Damato]

forgot to include the list in my reply...

On Fri, Nov 19, 2010 at 12:27 PM, Petr Machata <pmachata at redhat.com> wrote:
> 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.

your feedback makes sense. i've made these changes and rebased them on
my test branch.

> 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.

good catch. i've fixed this and rebased it, as well.

> 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

i pulled these fixes into my test branch, as well.

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

thanks for the code review.