From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45194) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XHEEH-00070S-9I for qemu-devel@nongnu.org; Tue, 12 Aug 2014 11:43:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XHEEA-0002oC-Ow for qemu-devel@nongnu.org; Tue, 12 Aug 2014 11:43:17 -0400 Message-ID: <53EA3604.5020402@redhat.com> Date: Tue, 12 Aug 2014 17:43:00 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: <53DE5538.1020701@gmail.com> In-Reply-To: <53DE5538.1020701@gmail.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] dump.c: Fix memory leak issue in cleanup processing for dump_init() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chen Gang Cc: qemu-trivial@nongnu.org, Michael Tokarev , qemu-devel@nongnu.org, agraf@suse.de, qiaonuohan@cn.fujitsu.com, pbonzini@redhat.com, lcapitulino@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 > --- > 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