[PATCH wayland 1/2] scanner: refactor creating objects and get rid of leaks
Marek Chalupa
mchqwerty at gmail.com
Wed Jul 22 22:36:55 PDT 2015
On 07/22/2015 07:43 PM, Derek Foreman wrote:
> On 22/07/15 02:25 AM, Marek Chalupa wrote:
>> 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.
>
> Yup, you're obviously right. Sorry for the line noise.
>
> I guess maybe it's nicer if the free_foo() functions have the same
> semantics as free(), but I leave that up to you. I'm ok with it either way.
>
Since in this case it is valid for description to be NULL,
free_description() should count with that, so yes, it is probably better.
>>> (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
>
Thanks,
Marek
More information about the wayland-devel
mailing list