[PATCH 3/5] Move the memory allocation of entry buffers to a separate function

[Dirk Hohndel]

I'm not sure I like this change. It seems to make the code more
complicated in order to improve memleak debugging. I am a little less
worried about memleaks in the parser and more concerned about
readability and debuggability of the code. But could be convinced
otherwise.

Linus, what do you think?

/D

"Lubomir I. Ivanov" <neolit123 at gmail.com> writes:

> From: "Lubomir I. Ivanov" <neolit123 at gmail.com>
>
> parse-xml.c:
> Once an entry is about to be parsed to internal data (in entry()),
> we allocate a buffer and then pass it to the try_to_fill_* functions,
> which take care of the "name-matching". However this method makes it
> (notoriously) difficult for memory debuggers (such as dr.memory and valgrind)
> to find the actual type of memory leak (e.g. is it event or divetrip related),
> as the allocation preceeds the sorting.
>
> If we move the malloc() call itself into a separate function, which is then
> called from try_to_fill_*, suddenly the type of the memory leak becomes
> easily determinable from the backtrace.
>
> Signed-off-by: Lubomir I. Ivanov <neolit123 at gmail.com>
> ---
>  parse-xml.c | 49 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 29 insertions(+), 20 deletions(-)
>
> diff --git a/parse-xml.c b/parse-xml.c
> index f68474a..b7de1d5 100644
> --- a/parse-xml.c
> +++ b/parse-xml.c
> @@ -19,6 +19,20 @@ int verbose;
>  
>  struct dive_table dive_table;
>  
> +/* we use this as a separate function and call it from try_to_fill_*,
> + * so that memory leaks related to the parser can be debugged more easily.
> + * once a leak is detected at this location, the call stack will also indicate
> + * a try_to_fill_* function. */
> +static char *alloc_fill_buffer(int size, const char* raw)
> +{
> +	char *buf = malloc(size + 1);
> +	if (!buf)
> +		exit(1);
> +	memcpy(buf, raw, size);
> +	buf[size] = 0;
> +	return buf;
> +}
> +
>  /*
>   * Add a dive into the dive_table array
>   */
> @@ -592,10 +606,10 @@ static void eventtime(char *buffer, void *_duration)
>  		duration->seconds += cur_sample->time.seconds;
>  }
>  
> -static void try_to_fill_event(const char *name, char *buf)
> +static void try_to_fill_event(const char *name, int size, const char *raw)
>  {
> +	char *buf = alloc_fill_buffer(size, raw);
>  	int len = strlen(name);
> -
>  	start_match("event", name, buf);
>  	if (MATCH(".event", utf8_string, &cur_event.name))
>  		return;
> @@ -613,10 +627,10 @@ static void try_to_fill_event(const char *name, char *buf)
>  }
>  
>  /* We're in the top-level dive xml. Try to convert whatever value to a dive value */
> -static void try_to_fill_dc(struct divecomputer *dc, const char *name, char *buf)
> +static void try_to_fill_dc(struct divecomputer *dc, const char *name, int size, const char *raw)
>  {
> +	char *buf = alloc_fill_buffer(size, raw);
>  	int len = strlen(name);
> -
>  	start_match("divecomputer", name, buf);
>  
>  	if (MATCH(".date", divedate, &dc->when))
> @@ -634,8 +648,9 @@ static void try_to_fill_dc(struct divecomputer *dc, const char *name, char *buf)
>  }
>  
>  /* We're in samples - try to convert the random xml value to something useful */
> -static void try_to_fill_sample(struct sample *sample, const char *name, char *buf)
> +static void try_to_fill_sample(struct sample *sample, const char *name, int size, const char *raw)
>  {
> +	char *buf = alloc_fill_buffer(size, raw);
>  	int len = strlen(name);
>  
>  	start_match("sample", name, buf);
> @@ -837,8 +852,9 @@ static void gps_location(char *buffer, void *_dive)
>  }
>  
>  /* We're in the top-level dive xml. Try to convert whatever value to a dive value */
> -static void try_to_fill_dive(struct dive *dive, const char *name, char *buf)
> +static void try_to_fill_dive(struct dive *dive, const char *name, int size, const char *raw)
>  {
> +	char *buf = alloc_fill_buffer(size, raw);
>  	int len = strlen(name);
>  
>  	start_match("dive", name, buf);
> @@ -945,10 +961,10 @@ static void try_to_fill_dive(struct dive *dive, const char *name, char *buf)
>  }
>  
>  /* We're in the top-level trip xml. Try to convert whatever value to a trip value */
> -static void try_to_fill_trip(dive_trip_t **dive_trip_p, const char *name, char *buf)
> +static void try_to_fill_trip(dive_trip_t **dive_trip_p, const char *name, int size, const char *raw)
>  {
> +	char *buf = alloc_fill_buffer(size, raw);
>  	int len = strlen(name);
> -
>  	start_match("trip", name, buf);
>  
>  	dive_trip_t *dive_trip = *dive_trip_p;
> @@ -1134,33 +1150,26 @@ static void divecomputer_end(void)
>  
>  static void entry(const char *name, int size, const char *raw)
>  {
> -	char *buf = malloc(size+1);
> -
> -	if (!buf)
> -		return;
> -	memcpy(buf, raw, size);
> -	buf[size] = 0;
>  	if (cur_event.active) {
> -		try_to_fill_event(name, buf);
> +		try_to_fill_event(name, size, raw);
>  		return;
>  	}
>  	if (cur_sample) {
> -		try_to_fill_sample(cur_sample, name, buf);
> +		try_to_fill_sample(cur_sample, name, size, raw);
>  		return;
>  	}
>  	if (cur_dc) {
> -		try_to_fill_dc(cur_dc, name, buf);
> +		try_to_fill_dc(cur_dc, name, size, raw);
>  		return;
>  	}
>  	if (cur_dive) {
> -		try_to_fill_dive(cur_dive, name, buf);
> +		try_to_fill_dive(cur_dive, name, size, raw);
>  		return;
>  	}
>  	if (cur_trip) {
> -		try_to_fill_trip(&cur_trip, name, buf);
> +		try_to_fill_trip(&cur_trip, name, size, raw);
>  		return;
>  	}
> -	free(buf);
>  }
>  
>  static const char *nodename(xmlNode *node, char *buf, int len)
> -- 
> 1.7.11.msysgit.0
>
> _______________________________________________
> subsurface mailing list
> subsurface at hohndel.org
> http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface

-- 
Dirk Hohndel
Intel Open Source Technology Center