Re: [PATCH] chdir-notify: UNLEAK registrated callback entries

[Junio C Hamano <gitster@pobox.com>]

René Scharfe <l.s.r@web.de> writes:

> Right, avoiding list.h like below lets Valgrind classify the memory as
> "still reachable", without UNLEAK.  Not sure if it's worth it, though.
>
> Note my somewhat concerning knee-jerk reaction to write some macros. ;)

We have used such 3-tuple of <vector, alloc, nr> in many places from
very early time, and these macros might be worth using if we were to
rewrite all of them.

As to this particular instance, given that the entries is either a
list or a vector of at most one element in practice, I do not think
it matters very much either way.  Just a single UNLEAK is certainly
simpler ;-)

Thanks.




>  struct chdir_notify_entry {
>  	const char *name;
>  	chdir_notify_callback cb;
>  	void *data;
> -	struct list_head list;
>  };
> -static LIST_HEAD(chdir_notify_entries);
> +
> +#define VECTOR_TYPE(elemtype) struct { elemtype *v; size_t alloc, nr; }
> +#define VECTOR_FOR_EACH(x, p) for (p = (x)->v; p < (x)->v + (x)->nr; p++)
> +#define VECTOR_NEW_ENTRY(x, p) do {			\
> +	ALLOC_GROW_BY((x)->v, (x)->nr, 1, (x)->alloc);	\
> +	p = (x)->v + (x)->nr - 1;			\
> +} while (0)
> +
> +static VECTOR_TYPE(struct chdir_notify_entry) chdir_notify_entries;
>
>  void chdir_notify_register(const char *name,
>  			   chdir_notify_callback cb,
>  			   void *data)
>  {
> -	struct chdir_notify_entry *e = xmalloc(sizeof(*e));
> -	UNLEAK(e);
> +	struct chdir_notify_entry *e;
> +
> +	VECTOR_NEW_ENTRY(&chdir_notify_entries, e);
>  	e->name = name;
>  	e->cb = cb;
>  	e->data = data;
> -	list_add_tail(&e->list, &chdir_notify_entries);
>  }
>
>  static void reparent_cb(const char *name,
> @@ -52,7 +58,7 @@ void chdir_notify_reparent(const char *name, char **path)
>  int chdir_notify(const char *new_cwd)
>  {
>  	struct strbuf old_cwd = STRBUF_INIT;
> -	struct list_head *pos;
> +	struct chdir_notify_entry *e;
>
>  	if (strbuf_getcwd(&old_cwd) < 0)
>  		return -1;
> @@ -67,11 +73,8 @@ int chdir_notify(const char *new_cwd)
>  			 "setup: chdir from '%s' to '%s'",
>  			 old_cwd.buf, new_cwd);
>
> -	list_for_each(pos, &chdir_notify_entries) {
> -		struct chdir_notify_entry *e =
> -			list_entry(pos, struct chdir_notify_entry, list);
> +	VECTOR_FOR_EACH(&chdir_notify_entries, e)
>  		e->cb(e->name, old_cwd.buf, new_cwd, e->data);
> -	}
>
>  	strbuf_release(&old_cwd);
>  	return 0;
> --
> 2.29.2