[PATCH wayland 1/2] scanner: refactor creating objects and get rid of leaks

Derek Foreman derekf at osg.samsung.com
Fri Jul 17 14:02:47 PDT 2015


On 16/07/15 06:59 AM, Marek Chalupa wrote:
> Create functions for structures allocation (instead of inlining it into
> the code) and free the objects after we don't use them anymore.
> 
> Signed-off-by: Marek Chalupa <mchqwerty at gmail.com>

I think this is quite nice, but I'd like to see the leak fixes in a
second patch, and no functional changes in the refactor patch.

Also,
if (foo)
	free(foo);

might as well be
free(foo);

since free(NULL); is totally fine.  I think the result looks cleaner.

(No further comments below)

> ---
>  src/scanner.c | 266 +++++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 209 insertions(+), 57 deletions(-)
> 
> diff --git a/src/scanner.c b/src/scanner.c
> index 7d8cfb9..c652612 100644
> --- a/src/scanner.c
> +++ b/src/scanner.c
> @@ -1,6 +1,7 @@
>  /*
>   * Copyright © 2008-2011 Kristian Høgsberg
>   * Copyright © 2011 Intel Corporation
> + * Copyright © 2015 Red Hat, Inc.
>   *
>   * Permission is hereby granted, free of charge, to any person obtaining
>   * a copy of this software and associated documentation files (the
> @@ -316,6 +317,185 @@ is_nullable_type(struct arg *arg)
>  }
>  
>  static void
> +free_description(struct description *desc)
> +{
> +	free(desc->summary);
> +	free(desc->text);
> +
> +	free(desc);
> +}
> +
> +static struct message *
> +create_message(struct location loc, const char *name)
> +{
> +	struct message *message;
> +
> +	message = xmalloc(sizeof *message);
> +	message->loc = loc;
> +	message->name = xstrdup(name);
> +	message->uppercase_name = uppercase_dup(name);
> +	wl_list_init(&message->arg_list);
> +	message->arg_count = 0;
> +	message->new_id_count = 0;
> +	message->description = NULL;
> +
> +	return message;
> +}
> +
> +static struct arg *
> +create_arg(const char *name, const char *type)
> +{
> +	struct arg *arg;
> +
> +	arg = xmalloc(sizeof *arg);
> +	arg->name = xstrdup(name);
> +	arg->summary = NULL;
> +	arg->interface_name = NULL;
> +
> +	if (strcmp(type, "int") == 0)
> +		arg->type = INT;
> +	else if (strcmp(type, "uint") == 0)
> +		arg->type = UNSIGNED;
> +	else if (strcmp(type, "fixed") == 0)
> +		arg->type = FIXED;
> +	else if (strcmp(type, "string") == 0)
> +		arg->type = STRING;
> +	else if (strcmp(type, "array") == 0)
> +		arg->type = ARRAY;
> +	else if (strcmp(type, "fd") == 0)
> +		arg->type = FD;
> +	else if (strcmp(type, "new_id") == 0)
> +		arg->type = NEW_ID;
> +	else if (strcmp(type, "object") == 0)
> +		arg->type = OBJECT;
> +	else
> +		return NULL;
> +
> +	return arg;
> +}
> +
> +static void
> +free_arg(struct arg *arg)
> +{
> +	free(arg->name);
> +	free(arg->interface_name);
> +	free(arg->summary);
> +	free(arg);
> +}
> +
> +static void
> +free_message(struct message *message)
> +{
> +	struct arg *a, *a_next;
> +
> +	free(message->name);
> +	free(message->uppercase_name);
> +	if (message->description)
> +		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)
> +{
> +	struct enumeration *enumeration;
> +
> +	enumeration = xmalloc(sizeof *enumeration);
> +	enumeration->name = xstrdup(name);
> +	enumeration->uppercase_name = uppercase_dup(name);
> +	enumeration->description = NULL;
> +
> +	wl_list_init(&enumeration->entry_list);
> +
> +	return enumeration;
> +}
> +
> +static struct entry *
> +create_entry(const char *name, const char *value)
> +{
> +	struct entry *entry;
> +
> +	entry = xmalloc(sizeof *entry);
> +	entry->name = xstrdup(name);
> +	entry->uppercase_name = uppercase_dup(name);
> +	entry->value = xstrdup(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);
> +
> +	if (enumeration->description)
> +		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)
> +{
> +	struct interface *interface;
> +
> +	interface = xmalloc(sizeof *interface);
> +	interface->loc = loc;
> +	interface->name = xstrdup(name);
> +	interface->uppercase_name = uppercase_dup(name);
> +	interface->version = version;
> +	interface->description = NULL;
> +	interface->since = 1;
> +	wl_list_init(&interface->request_list);
> +	wl_list_init(&interface->event_list);
> +	wl_list_init(&interface->enumeration_list);
> +
> +	return interface;
> +}
> +
> +static void
> +free_interface(struct interface *interface)
> +{
> +	struct message *m, *next_m;
> +	struct enumeration *e, *next_e;
> +
> +	free(interface->name);
> +	free(interface->uppercase_name);
> +	if (interface->description)
> +		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;
> @@ -376,32 +556,16 @@ start_element(void *data, const char *element_name, const char **atts)
>  		if (version == 0)
>  			fail(&ctx->loc, "no interface version given");
>  
> -		interface = xmalloc(sizeof *interface);
> -		interface->loc = ctx->loc;
> -		interface->name = xstrdup(name);
> -		interface->uppercase_name = uppercase_dup(name);
> -		interface->version = version;
> -		interface->description = NULL;
> -		interface->since = 1;
> -		wl_list_init(&interface->request_list);
> -		wl_list_init(&interface->event_list);
> -		wl_list_init(&interface->enumeration_list);
> +		interface = create_interface(ctx->loc, name, version);
> +		ctx->interface = interface;
>  		wl_list_insert(ctx->protocol->interface_list.prev,
>  			       &interface->link);
> -		ctx->interface = interface;
>  	} else if (strcmp(element_name, "request") == 0 ||
>  		   strcmp(element_name, "event") == 0) {
>  		if (name == NULL)
>  			fail(&ctx->loc, "no request name given");
>  
> -		message = xmalloc(sizeof *message);
> -		message->loc = ctx->loc;
> -		message->name = xstrdup(name);
> -		message->uppercase_name = uppercase_dup(name);
> -		wl_list_init(&message->arg_list);
> -		message->arg_count = 0;
> -		message->new_id_count = 0;
> -		message->description = NULL;
> +		message = create_message(ctx->loc, name);
>  
>  		if (strcmp(element_name, "request") == 0)
>  			wl_list_insert(ctx->interface->request_list.prev,
> @@ -440,28 +604,9 @@ start_element(void *data, const char *element_name, const char **atts)
>  		if (name == NULL)
>  			fail(&ctx->loc, "no argument name given");
>  
> -		arg = xmalloc(sizeof *arg);
> -		arg->name = xstrdup(name);
> -
> -		if (strcmp(type, "int") == 0)
> -			arg->type = INT;
> -		else if (strcmp(type, "uint") == 0)
> -			arg->type = UNSIGNED;
> -		else if (strcmp(type, "fixed") == 0)
> -			arg->type = FIXED;
> -		else if (strcmp(type, "string") == 0)
> -			arg->type = STRING;
> -		else if (strcmp(type, "array") == 0)
> -			arg->type = ARRAY;
> -		else if (strcmp(type, "fd") == 0)
> -			arg->type = FD;
> -		else if (strcmp(type, "new_id") == 0) {
> -			arg->type = NEW_ID;
> -		} else if (strcmp(type, "object") == 0) {
> -			arg->type = OBJECT;
> -		} else {
> +		arg = create_arg(name, type);
> +		if (!arg)
>  			fail(&ctx->loc, "unknown type (%s)", type);
> -		}
>  
>  		switch (arg->type) {
>  		case NEW_ID:
> @@ -472,8 +617,6 @@ start_element(void *data, const char *element_name, const char **atts)
>  		case OBJECT:
>  			if (interface_name)
>  				arg->interface_name = xstrdup(interface_name);
> -			else
> -				arg->interface_name = NULL;
>  			break;
>  		default:
>  			if (interface_name != NULL)
> @@ -491,7 +634,6 @@ start_element(void *data, const char *element_name, const char **atts)
>  		if (allow_null != NULL && !is_nullable_type(arg))
>  			fail(&ctx->loc, "allow-null is only valid for objects, strings, and arrays");
>  
> -		arg->summary = NULL;
>  		if (summary)
>  			arg->summary = xstrdup(summary);
>  
> @@ -501,12 +643,7 @@ start_element(void *data, const char *element_name, const char **atts)
>  		if (name == NULL)
>  			fail(&ctx->loc, "no enum name given");
>  
> -		enumeration = xmalloc(sizeof *enumeration);
> -		enumeration->name = xstrdup(name);
> -		enumeration->uppercase_name = uppercase_dup(name);
> -		enumeration->description = NULL;
> -		wl_list_init(&enumeration->entry_list);
> -
> +		enumeration = create_enumeration(name);
>  		wl_list_insert(ctx->interface->enumeration_list.prev,
>  			       &enumeration->link);
>  
> @@ -515,10 +652,8 @@ start_element(void *data, const char *element_name, const char **atts)
>  		if (name == NULL)
>  			fail(&ctx->loc, "no entry name given");
>  
> -		entry = xmalloc(sizeof *entry);
> -		entry->name = xstrdup(name);
> -		entry->uppercase_name = uppercase_dup(name);
> -		entry->value = xstrdup(value);
> +		entry = create_entry(name, value);
> +
>  		if (summary)
>  			entry->summary = xstrdup(summary);
>  		else
> @@ -1049,7 +1184,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;
> @@ -1102,7 +1237,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);
>  
> @@ -1116,6 +1251,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"
> @@ -1231,7 +1368,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;
>  
> @@ -1266,7 +1403,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");
> @@ -1289,9 +1426,22 @@ 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);
> +	if (protocol->description)
> +		free_description(protocol->description);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	struct parse_context ctx;
> @@ -1415,5 +1565,7 @@ int main(int argc, char *argv[])
>  			break;
>  	}
>  
> +	free_protocol(&protocol);
> +
>  	return 0;
>  }
> 



More information about the wayland-devel mailing list