Re: [Qemu-devel] [PATCH] dump.c: Fix memory leak issue in cleanup processing for dump_init()

[Laszlo Ersek <lersek@redhat.com>]

On 08/03/14 17:28, Chen Gang wrote:
> In dump_init(), when failure occurs, need notice about 'fd' and memory
> mapping. So call dump_cleanup() for it (need let all initializations at
> front).
> 
> Also simplify dump_cleanup(): remove redundant 'ret' and redundant 'fd'
> checking.
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  dump.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index ce646bc..71d3e94 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -71,18 +71,14 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
>  
>  static int dump_cleanup(DumpState *s)
>  {
> -    int ret = 0;
> -
>      guest_phys_blocks_free(&s->guest_phys_blocks);
>      memory_mapping_list_free(&s->list);
> -    if (s->fd != -1) {
> -        close(s->fd);
> -    }
> +    close(s->fd);
>      if (s->resume) {
>          vm_start();
>      }
>  
> -    return ret;
> +    return 0;
>  }
>  
>  static void dump_error(DumpState *s, const char *reason)
> @@ -1499,6 +1495,8 @@ static int dump_init(DumpState *s, int fd, bool has_format,
>      s->begin = begin;
>      s->length = length;
>  
> +    memory_mapping_list_init(&s->list);
> +
>      guest_phys_blocks_init(&s->guest_phys_blocks);
>      guest_phys_blocks_append(&s->guest_phys_blocks);
>  
> @@ -1526,7 +1524,6 @@ static int dump_init(DumpState *s, int fd, bool has_format,
>      }
>  
>      /* get memory mapping */
> -    memory_mapping_list_init(&s->list);
>      if (paging) {
>          qemu_get_guest_memory_mapping(&s->list, &s->guest_phys_blocks, &err);
>          if (err != NULL) {
> @@ -1622,12 +1619,7 @@ static int dump_init(DumpState *s, int fd, bool has_format,
>      return 0;
>  
>  cleanup:
> -    guest_phys_blocks_free(&s->guest_phys_blocks);
> -
> -    if (s->resume) {
> -        vm_start();
> -    }
> -
> +    dump_cleanup(s);
>      return -1;
>  }
>  
> 

The patch is not trivial at all, because the lifecycles of the affected
resources are not trivial.

The commit message is an abomination. If you want to contribute to open
source, please learn proper English, and make a *serious* effort to
document your changes. Don't expect earlier contributors to have any
working knowledge about a file they have *maybe* touched several months
ago. People have to swap out a whole load of crap from their minds, and
swap in the old crap, to understand your patch. Help them by writing
good commit messages.

That said, no matter how annoying this submission is, my conscience
didn't allow me to let it go ignored, so I spent the time and made an
effort to track the lifetimes of "s->fd" and "s->list" in, and around,
dump_init().

The patch seems correct. That's your only excuse for the loss of gray
matter I've suffered while parsing your commit message.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>