[systemd-devel] systemd coverity
Zbigniew Jędrzejewski-Szmek
zbyszek at in.waw.pl
Thu Aug 23 07:01:13 PDT 2012
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
More information about the systemd-devel
mailing list