[PATCH] libdwfl: Add dwfl_getthread and dwfl_getthread_frames.

Mark Wielaard mjw at redhat.com
Mon Dec 23 21:58:28 UTC 2013


On Mon, 2013-12-23 at 18:53 +0100, Jan Kratochvil wrote:
> > Updated patch attached.
> 
> Fine with that with the comments below.

Pushed to master with the comments fixed.

> > +static int
> > +get_one_thread_cb (Dwfl_Thread *thread, void *arg)
> > +{
> > +  struct one_arg *oa = (struct one_arg *) arg;
> > +  if (! oa->seen && INTUSE(dwfl_thread_tid) (thread) == oa->tid)
> > +    {
> > +      oa->seen = true;
> > +      return oa->callback (thread, oa->arg);
> 
> Maybe it was intentionall to keep the code simple - but if oa->callback
> returns DWARF_CB_OK then it will needlessly continue iterating the threads
> despite it can never find that TID again.

Yes I missed that. Added a ret argument to struct one_arg to catch to
actual callback return value and return DWARF_CB_ABORT getthread then
returns that value when it sees DWARF_CB_ABORT with seen == true.

> > +/* Note not currently exported, will be when there are more Dwfl_Thread
> > +   properties to query.  Use dwfl_getthread_frames for now directly.  */
> > +static int
> > +dwfl_getthread (Dwfl *dwfl, pid_t tid,
> 
> It is no longer public so I think it should not have dwfl_* prefix.

Removed dwfl_ prefix.

> > +		int (*callback) (Dwfl_Thread *thread, void *arg),
> > +		void *arg)
> > +{
> > +  Dwfl_Process *process = dwfl->process;
> > +  if (process == NULL)
> 
> As the function is no longer public this can be rather an assert.

It could, but since we do want to make this public at one point I kept
it as is, so we don't forget the actual logic needed.

> > +   struct one_arg oa = { .tid = tid, .callback = callback,
> > +			 .arg = arg, .seen = false };
> > +   int err = INTUSE(dwfl_getthreads) (dwfl, get_one_thread_cb, &oa);
> > +   if (err == DWARF_CB_OK && ! oa.seen)
> 
> __libdwfl_seterrno is not called.

Set to DWFL_E_ERRNO with errno = ESRCH.
Added the same to pid_getthread which also failed to set the error.

> > +  /* Called to get a specific thread.  Returns true if there is a
> > +     thread with the given thread id number, returns false if no such
> > +     thread exists and may set dwfl_errno in that case.  THREAD_ARGP
> 
> s/may/it will/.  Setting errno has to be mandatory, not voluntary.

Changed.

Thanks,

Mark




More information about the elfutils-devel mailing list