[PATCH wayland v3 2/3] scanner: get rid of leaks

[Bryce Harrington]

On Thu, Jul 30, 2015 at 03:07:21PM +0200, Marek Chalupa wrote:
> Free all the memory we have allocated during running.
> 
> v2.: split creating objects and getting rid of leaks
>      into two patches
> 
>      move check for NULL description into free_description
> 
> v3.: rebase after previous patch fixes
> 
> Signed-off-by: Marek Chalupa <mchqwerty at gmail.com>

One stylistic comment below, but it can be handled in follow-up.
Everything else looks good to me, so:

Reviewed-by: Bryce Harrington <bryce at osg.samsung.com>

> ---
>  src/scanner.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 102 insertions(+), 4 deletions(-)
> 
> diff --git a/src/scanner.c b/src/scanner.c
> index da33818..19755ec 100644
> --- a/src/scanner.c
> +++ b/src/scanner.c
> @@ -333,6 +333,15 @@ create_message(struct location loc, const char *name)
>  	return message;
>  }
>  
> +static void
> +free_arg(struct arg *arg)
> +{
> +	free(arg->name);
> +	free(arg->interface_name);
> +	free(arg->summary);
> +	free(arg);
> +}
> +
>  static struct arg *
>  create_arg(const char *name)
>  {
> @@ -371,6 +380,33 @@ set_arg_type(struct arg *arg, const char *type)
>  	return true;
>  }
>  
> +static void
> +free_description(struct description *desc)
> +{
> +	if (!desc)
> +		return;

I know this was added in response to review comments from the original
patch.  Fwiw, I tend to agree, this makes the behavior of
free_description() work more like free() when given an already-freed
object.

However, all the other free_* routines here do not have this behavior;
all assume they're freeing a non-NULL object.

So, these routines could be made consistent by having them all do a null
ptr check on their arg.  That said, I know that often in Wayland we
deliberately do *not* do these types of checks as a matter of policy; in
which case, if we're going to follow that policy with the other
routines, perhaps it should be followed with this one as well?

Anyway, not a biggie but maybe could be a nice cleanup followup.

> +	free(desc->summary);
> +	free(desc->text);
> +
> +	free(desc);
> +}
> +
> +static void
> +free_message(struct message *message)
> +{
> +	struct arg *a, *a_next;
> +
> +	free(message->name);
> +	free(message->uppercase_name);
> +	free_description(message->description);
> +
> +	wl_list_for_each_safe(a, a_next, &message->arg_list, link)
> +		free_arg(a);
> +
> +	free(message);
> +}
> +
>  static struct enumeration *
>  create_enumeration(const char *name)
>  {
> @@ -399,6 +435,32 @@ create_entry(const char *name, const char *value)
>  	return entry;
>  }
>  
> +static void
> +free_entry(struct entry *entry)
> +{
> +	free(entry->name);
> +	free(entry->uppercase_name);
> +	free(entry->value);
> +	free(entry->summary);
> +
> +	free(entry);
> +}
> +
> +static void
> +free_enumeration(struct enumeration *enumeration)
> +{
> +	struct entry *e, *e_next;
> +
> +	free(enumeration->name);
> +	free(enumeration->uppercase_name);
> +	free_description(enumeration->description);
> +
> +	wl_list_for_each_safe(e, e_next, &enumeration->entry_list, link)
> +		free_entry(e);
> +
> +	free(enumeration);
> +}
> +
>  static struct interface *
>  create_interface(struct location loc, const char *name, int version)
>  {
> @@ -419,6 +481,26 @@ create_interface(struct location loc, const char *name, int version)
>  }
>  
>  static void
> +free_interface(struct interface *interface)
> +{
> +	struct message *m, *next_m;
> +	struct enumeration *e, *next_e;
> +
> +	free(interface->name);
> +	free(interface->uppercase_name);
> +	free_description(interface->description);
> +
> +	wl_list_for_each_safe(m, next_m, &interface->request_list, link)
> +		free_message(m);
> +	wl_list_for_each_safe(m, next_m, &interface->event_list, link)
> +		free_message(m);
> +	wl_list_for_each_safe(e, next_e, &interface->enumeration_list, link)
> +		free_enumeration(e);
> +
> +	free(interface);
> +}
> +
> +static void
>  start_element(void *data, const char *element_name, const char **atts)
>  {
>  	struct parse_context *ctx = data;
> @@ -1107,7 +1189,7 @@ get_include_name(bool core, enum side side)
>  static void
>  emit_header(struct protocol *protocol, enum side side)
>  {
> -	struct interface *i;
> +	struct interface *i, *i_next;
>  	struct wl_array types;
>  	const char *s = (side == SERVER) ? "SERVER" : "CLIENT";
>  	char **p, *prev;
> @@ -1160,7 +1242,7 @@ emit_header(struct protocol *protocol, enum side side)
>  
>  	printf("\n");
>  
> -	wl_list_for_each(i, &protocol->interface_list, link) {
> +	wl_list_for_each_safe(i, i_next, &protocol->interface_list, link) {
>  
>  		emit_enumerations(i);
>  
> @@ -1174,6 +1256,8 @@ emit_header(struct protocol *protocol, enum side side)
>  			emit_opcodes(&i->request_list, i);
>  			emit_stubs(&i->request_list, i);
>  		}
> +
> +		free_interface(i);
>  	}
>  
>  	printf("#ifdef  __cplusplus\n"
> @@ -1289,7 +1373,7 @@ emit_messages(struct wl_list *message_list,
>  static void
>  emit_code(struct protocol *protocol)
>  {
> -	struct interface *i;
> +	struct interface *i, *next;
>  	struct wl_array types;
>  	char **p, *prev;
>  
> @@ -1324,7 +1408,7 @@ emit_code(struct protocol *protocol)
>  	}
>  	printf("};\n\n");
>  
> -	wl_list_for_each(i, &protocol->interface_list, link) {
> +	wl_list_for_each_safe(i, next, &protocol->interface_list, link) {
>  
>  		emit_messages(&i->request_list, i, "requests");
>  		emit_messages(&i->event_list, i, "events");
> @@ -1347,9 +1431,21 @@ emit_code(struct protocol *protocol)
>  			printf("\t0, NULL,\n");
>  
>  		printf("};\n\n");
> +
> +		/* we won't need it any further */
> +		free_interface(i);
>  	}
>  }
>  
> +static void
> +free_protocol(struct protocol *protocol)
> +{
> +	free(protocol->name);
> +	free(protocol->uppercase_name);
> +	free(protocol->copyright);
> +	free_description(protocol->description);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	struct parse_context ctx;
> @@ -1473,5 +1569,7 @@ int main(int argc, char *argv[])
>  			break;
>  	}
>  
> +	free_protocol(&protocol);
> +
>  	return 0;
>  }
> -- 
> 2.4.3
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel