On Tue, 2013-10-22 at 22:32 +0200, Jan Kratochvil wrote:
On Mon, 21 Oct 2013 21:19:40 +0200, Jan Kratochvil wrote:
> On Sat, 19 Oct 2013 03:05:23 +0200, Mark Wielaard wrote:
> > Again mostly nitpicks. The only real issue I would like you to
> > think/comment on is around the dwfl_thread_next comments.
>
> Maybe you want threads to be iterated through callbacks?
Implemented it in the attached diff and checked into jankratochvil/unwindx86.
I like it. It seems to simplify the code.
@@ -260,47 +246,50 @@ dwfl_frame_thread (Dwfl_Frame *state)
}
INTDEF(dwfl_frame_thread)
-Dwfl_Thread *
-dwfl_next_thread (Dwfl *dwfl, Dwfl_Thread *prev_thread)
+int
+dwfl_getthreads (Dwfl *dwfl, int (*callback) (Dwfl_Thread *thread, void *arg),
+ void *arg)
[...]
+ nthread->tid = process->callbacks->next_thread (dwfl, nthread,
+ process->callbacks_arg,
+ &nthread->callbacks_arg);
+ if (nthread->tid < 0)
+ {
+ Dwfl_Error saved_errno = dwfl_errno ();
+ thread_free (nthread);
+ __libdwfl_seterrno (saved_errno);
+ return -1;
+ }
thread_free doesn't set dwfl_errno, so should be save to call without
saving/restoring it.
I think you can now also simplify the Dwfl_Thread_Callbacks next_thread.
It doesn't really seem to need the Dwfl_Thread that is being constructed
since it doesn't seem contain anything that helps with constructing the
next one. So I would change it to something like:
/* Called to iterate through threads. Returns next TID (thread ID) on
success, a negative number on failure and zero if there are no more
threads. dwfl_errno () should be set if negative number has been
returned. *THREAD_ARGP will be NULL on the first call and may be
optionally set by the implementation. On next calls **THREAD_ARGP
will point to the *THREAD_ARGP value set last time. THREAD_ARGP is
never NULL. *THREAD_ARGP will be passed to set_initial_registers
or thread_detach callbacks together with Dwfl_Thread *thread.
This method must not be NULL. */
pid_t (*next_thread) (Dwfl *dwfl, void *dwfl_arg, void **thread_argp)
__nonnull_attribute__ (1, 2);
That way the callback can keep any state it wants between next_thread
calls through *THREAD_ARGP if it wants.
Does that make sense?
Cheers,
Mark