forgot to include the list in my reply... On Fri, Nov 19, 2010 at 12:27 PM, Petr Machata <[email protected]> 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. _______________________________________________ Ltrace-devel mailing list [email protected] http://lists.alioth.debian.org/mailman/listinfo/ltrace-devel
