Re: [Qemu-devel] [PATCH 2/6] qht: add qht_iter_remove

["Emilio G. Cota" <cota@braap.org>]

On Fri, Sep 07, 2018 at 15:51:12 +0100, Alex Bennée wrote:
> 
> Emilio G. Cota <cota@braap.org> writes:
> 
> > This currently has no users, but the use case is so common that I
> > think we must support it.
> >
> > Note that without the appended we cannot safely remove a set of
> > elements; a 2-step approach (i.e. qht_iter first, keep track of
> > the to-be-deleted elements, and then a bunch of qht_remove calls)
> > would be racy, since between the iteration and the removals other
> > threads might insert additional elements.
> >
> > Signed-off-by: Emilio G. Cota <cota@braap.org>
> > ---
> >  include/qemu/qht.h | 19 ++++++++++++
> >  util/qht.c         | 74 +++++++++++++++++++++++++++++++++++++++++-----
> >  2 files changed, 85 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/qemu/qht.h b/include/qemu/qht.h
> > index 1fb9116fa0..91bc9b00cf 100644
> > --- a/include/qemu/qht.h
> > +++ b/include/qemu/qht.h
> > @@ -44,6 +44,8 @@ struct qht_stats {
> >
> >  typedef bool (*qht_lookup_func_t)(const void *obj, const void *userp);
> >  typedef void (*qht_iter_func_t)(struct qht *ht, void *p, uint32_t h, void *up);
> > +typedef bool (*qht_iter_bool_func_t)(struct qht *ht, void *p, uint32_t h,
> > +                                     void *up);
> >
> >  #define QHT_MODE_AUTO_RESIZE 0x1 /* auto-resize when heavily loaded */
> >
> > @@ -178,9 +180,26 @@ bool qht_resize(struct qht *ht, size_t n_elems);
> >   *
> >   * Each time it is called, user-provided @func is passed a pointer-hash pair,
> >   * plus @userp.
> > + *
> > + * Note: @ht cannot be accessed from @func
> 
> I'm confused by this comment. If @ht cannot be accessed (or shouldn't be
> accessed) from @func then why are we passing it?

We could probably do without. My original thinking was to pass the *ht
to tell the iter function which ht his k-v pair comes from.
But that's not a common use case, so we should remove it to
simplify the interface.

I think the comment is important right now; note that we can't even
perform a lookup from the callback without deadlock.

So I'll take your R-b for this patch and remove the *ht in
another patch.

Thanks,

		Emilio