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

Derek Foreman derekf at osg.samsung.com
Wed Jul 22 10:43:57 PDT 2015


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.

>> (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