[systemd-devel] systemd coverity

[Zbigniew Jędrzejewski-Szmek]

On 08/23/2012 03:11 PM, Daniel P. Berrange wrote:
> On Thu, Aug 23, 2012 at 03:04:23PM +0200, Zbigniew Jędrzejewski-Szmek wrote:
>> On 08/23/2012 02:36 PM, Lennart Poettering wrote:
>>> maybe we should add macros like:
>>>
>>>         #define _cleanup_free_ __attribute__((cleanup(freep)))
>>>         #define _cleanup_fclose_ __attribute__((cleanup(fclosep)))
>>>
>>> What do you think?
>> I personally think that this would be a welcome change like the
>> #pragma once cleanup.
>>
>> __attribute__(cleanup) goes all the way back to gcc 3.3, so there's
>> little reason not to use it.
>>
>>
>> On a related topic:
>> maybe gotos should be used more often for structured cleanup.
>> This might make the code slightly shorted, and also help avoid
>> mistakes.
>>
>> diff --git src/journal/sd-journal.c src/journal/sd-journal.c
>> index 0f7c02c..5e73a94 100644
>> --- src/journal/sd-journal.c
>> +++ src/journal/sd-journal.c
>> @@ -1418,37 +1418,33 @@ static sd_journal *journal_new(int flags, const
>> char *path) {
>>
>>          if (path) {
>>                  j->path = strdup(path);
>> -                if (!j->path) {
>> -                        free(j);
>> -                        return NULL;
>> -                }
>> +                if (!j->path)
>> +                        goto free_1;
>>          }
>>
>>          j->files = hashmap_new(string_hash_func, string_compare_func);
>> -        if (!j->files) {
>> -                free(j->path);
>> -                free(j);
>> -                return NULL;
>> -        }
>> +        if (!j->files)
>> +                goto free_2;
>>
>>          j->directories_by_path = hashmap_new(string_hash_func,
>> string_compare_func);
>> -        if (!j->directories_by_path) {
>> -                hashmap_free(j->files);
>> -                free(j->path);
>> -                free(j);
>> -                return NULL;
>> -        }
>> +        if (!j->directories_by_path)
>> +                goto free_3;
>>
>>          j->mmap = mmap_cache_new();
>> -        if (!j->mmap) {
>> -                hashmap_free(j->files);
>> -                hashmap_free(j->directories_by_path);
>> -                free(j->path);
>> -                free(j);
>> -                return NULL;
>> -        }
>> +        if (!j->mmap)
>> +                goto free_4;
>>
>>          return j;
>> +
>> +free_4:
>> +        hashmap_free(j->files);
>> +free_3:
>> +        hashmap_free(j->directories_by_path);
>> +free_2:
>> +        free(j->path);
>> +free_1:
>> +        free(j);
> 
> If you make sure that your pointer vars are all initialized to NULL,
> and that all "free" functions accept NULL, then you can collapse all
> those separate labels into one. This is much nicer, because then
> you don't need to go about re-numbering if you need to insert another
> goto in the middle of the function.
It is also necessary to make sure that j isn't null in the free_4-2.
But indeed they can be collapsed.

Zbyszek