[systemd-devel] systemd coverity

[Daniel P. Berrange]

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.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|