On Tue, Jun 26, 2018 at 04:17:37PM +0200, Willy Tarreau wrote:
> Hi Tim,
>
> On Tue, Jun 26, 2018 at 03:57:29PM +0200, Tim Duesterhus wrote:
> > diff --git a/src/stick_table.c b/src/stick_table.c
> > index 42946545..8e16830d 100644
> > --- a/src/stick_table.c
> > +++ b/src/stick_table.c
> > @@ -1596,8 +1596,10 @@ static int sample_conv_table_trackers(const struct
> > arg *arg_p, struct sample *sm
> > smp->data.type = SMP_T_SINT;
> > smp->data.u.sint = 0;
> >
> > - if (ts)
> > - smp->data.u.sint = ts->ref_cnt;
> > + if (!ts)
> > + return 1;
> > +
> > + smp->data.u.sint = ts->ref_cnt;
> >
> > stktable_release(t, ts);
> > return 1;
>
> Well, I don't understand, this does exactly the same as the current patch
Ah I just understood, I thought you meant the current patch doesn't work
but in fact it's that another functoin is affected as well, then it
totally makes sense. This one is even more serious because it's used
much more often.
Oh and my comment about thread safety is wrong, I read too fast, I thought
you were doing ref_cnt-- here *instead* of calling stktable_release(), I'm
sorry, I'll learn to read next time.
I *think* your patch is OK. I just hate having multiple exit points when
refcounts are involved, we're pretty sure that in a future version we'll
mess up with this one when adding something else. But we can slightly
adjust it to run "if (ts) { ... }" and that will be OK.
Thank you Tim!
Willy
