[DTrace-devel] [PATCH v2 04/11] htab: have dt_htab_destroy del all the elements

[Eugene Loh]

I'm struggling with this one.

On 10/20/21 2:53 PM, Nick Alcock wrote:
> This means that a dt_htab_destroy of a non-empy htab doesn't leak memory
> and leave all its former elements full of wild pointers.

s/empy/empty/

Were we leaking memory?  If so, what does this patch do to fix that?  I 
can see that the former elements will have their next/prev pointers set 
to NULL, dealing with those "wild pointers," but the patch does not free 
any memory.  Or, what am I missing?

> Also add some comments to dt_htab_delete given how many mistakes I made
> reading it.

Thanks for adding the comments, but I don't think that merits mention in 
the commit message.  Personal choice, I suppose.

> diff --git a/libdtrace/dt_htab.c b/libdtrace/dt_htab.c
>   void dt_htab_destroy(dtrace_hdl_t *dtp, dt_htab_t *htab)
>   {
> +	size_t		i;
> +
>   	if (!htab)
>   		return;
>   
> +	for (i = 0; i < htab->size; i++) {
> +		dt_hbucket_t	*bucket = htab->tab[i];
> +
> +		while (bucket && bucket->head)
> +			bucket->head = htab->ops->del(bucket->head,
> +			    bucket->head);
> +		free(bucket);
> +	}
> +
>   	dt_free(dtp, htab->tab);
>   	dt_free(dtp, htab);
>   }

Inside the "for i" loop, bucket does not change.  So, maybe replace
         while (bucket && bucket->head)
with
         if (bucket == NULL) continue;
         while (bucket->head)

More importantly, is the looping right?  You loop over slots.  For each 
slot, you choose a bucket, deleting its entries and then freeing the 
bucket.  But what if there are multiple buckets per slot?  Don't you 
have to chase bucket->next before freeing bucket?

> @@ -201,9 +216,16 @@ int dt_htab_delete(dt_htab_t *htab, void *entry)
>   	if (bucket == NULL)
>   		return -ENOENT;
>   
> +	/*
> +	 * Destroy the specified entry, now known to be in this bucket.
> +	 */
>   	head = htab->ops->del(bucket->head, entry);
>   	bucket->nentries--;
>   	htab->nentries--;
s/Destroy/Delete/ maybe?  To align better with the function name del()?