[PATCH wayland 1/2] scanner: refactor creating objects and get rid of leaks
Marek Chalupa
mchqwerty at gmail.com
Wed Jul 22 00:25:55 PDT 2015
Thanks for review,
On 07/17/2015 11:02 PM, Derek Foreman wrote:
> 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.
>
I'm not sure what places you think. There's only
if (description)
free_description(description)
because free_description dereferences the description, thus we need to
check if it is NULL. I could move the check into the function though.
> (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;
>> }
>>
>
Thanks,
Marek
More information about the wayland-devel
mailing list