On Wed, Jun 22, 2022 at 01:24:14AM +0300, Dmitry Kozlyuk wrote:
> 2022-06-21 14:28 (UTC-0700), Tyler Retzlaff:
> > On Tue, Jun 21, 2022 at 10:44:21PM +0300, Dmitry Kozlyuk wrote:
> > > 2022-06-21 11:51 (UTC-0700), Tyler Retzlaff:
> > > > > > +int
> > > > > > +rte_thread_join(rte_thread_t thread_id, unsigned long *value_ptr)
> > > > > > +{
> > > > > > + int ret = 0;
> > > > > > + void *res = NULL;
> > > > > > + void **pres = NULL;
> > > > > > +
> > > > > > + if (value_ptr != NULL)
> > > > > > + pres = &res;
> > > > > > +
> > > > > > + ret = pthread_join((pthread_t)thread_id.opaque_id, pres);
> > > > > > + if (ret != 0) {
> > > > > > + RTE_LOG(DEBUG, EAL, "pthread_join failed\n");
> > > > > > + return ret;
> > > > > > + }
> > > > > > +
> > > > > > + if (value_ptr != NULL && *pres != NULL)
> > > > > > + *value_ptr = *(unsigned long *)(*pres);
> > > > > > +
> > > > > > + return 0;
> > > > > > +}
> > > > >
> > > > > What makes *pres == NULL special?
> > > >
> > > > it's not clear what you mean, can you explain? maybe there is some
> > > > context i am missing from the original patch series?
> > >
> > > There's no previous context.
> > > After ptread_join(), *pres holds the return value of the thread routine.
> > > You only assign *value_ptr if value_ptr is not NULL (obviously correct)
> > > and if *pres != NULL, that is, if the thread returned a non-NULL value.
> > > But this value is opaque, why do you filter NULL?
> >
> > i don't think it is opaque here? unsigned long * value_ptr says we have
> > to store an integer. which leads to a discussion of what should get
> > stored at the value_ptr location if pthread_join() itself returns no
> > result but the caller of rte_thread_join() requests the result.
>
> There is no question. If `pthread_join()` fails, the function exits early
> and `*value_ptr` remains unmodified. If `pthread_join()` succeeds
> with a non-NULL second argument (`pres`), `*pres` aka `res` is always filled.
> NULL can be placed there too if that's what the thread routine has returned.
okay, discussed offline it was just my misunderstanding of impact on the
conditional block in the presence of the NULL check.
as discussed we'll rewrite the check as.
if (value_ptr != NULL)
*value_ptr = (uint32_t)(uintptr_t)res;
this doesn't double dereference pres as the original code was and
therefore the *pres NULL check is unnecessary.
thanks!
