On Fri, 11/27 10:48, Peter Xu wrote: > It might be a little bit confusing to do dump_cleanup() in these two > functions (and error prone, please see section below). A better way > is to do dump_cleanup() before dump finish, no matter whether dump > has succeeded or not. > > Meanwhile, this patch will also fix the potential memory leaks that > might happen when dump got errors within the process (e.g., when > write_dump_header() fails, it will skip calling dump_cleanup(), > which will leads to memory leak for list elements in DumpState).
I think when write_dump_header() returns a failure, it does call dump_cleanup(), in create_header{32,64}. But the changes of code in this patch look sane to me, and the new error handling is definitely looking better. Fam > > Signed-off-by: Peter Xu <pet...@redhat.com> > --- > dump.c | 78 > +++++++++++++++++++++++++++--------------------------------------- > 1 file changed, 32 insertions(+), 46 deletions(-) > > diff --git a/dump.c b/dump.c > index 78b7d84..445e739 100644 > --- a/dump.c > +++ b/dump.c > @@ -82,12 +82,6 @@ static int dump_cleanup(DumpState *s) > return 0; > } > > -static void dump_error(DumpState *s, const char *reason, Error **errp) > -{ > - dump_cleanup(s); > - error_setg(errp, "%s", reason); > -} > - > static int fd_write_vmcore(const void *buf, size_t size, void *opaque) > { > DumpState *s = opaque; > @@ -128,7 +122,7 @@ static void write_elf64_header(DumpState *s, Error **errp) > > ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s); > if (ret < 0) { > - dump_error(s, "dump: failed to write elf header", errp); > + error_setg(errp, "dump: failed to write elf header"); > } > } > > @@ -159,7 +153,7 @@ static void write_elf32_header(DumpState *s, Error **errp) > > ret = fd_write_vmcore(&elf_header, sizeof(elf_header), s); > if (ret < 0) { > - dump_error(s, "dump: failed to write elf header", errp); > + error_setg(errp, "dump: failed to write elf header"); > } > } > > @@ -182,7 +176,7 @@ static void write_elf64_load(DumpState *s, MemoryMapping > *memory_mapping, > > ret = fd_write_vmcore(&phdr, sizeof(Elf64_Phdr), s); > if (ret < 0) { > - dump_error(s, "dump: failed to write program header table", errp); > + error_setg(errp, "dump: failed to write program header table"); > } > } > > @@ -205,7 +199,7 @@ static void write_elf32_load(DumpState *s, MemoryMapping > *memory_mapping, > > ret = fd_write_vmcore(&phdr, sizeof(Elf32_Phdr), s); > if (ret < 0) { > - dump_error(s, "dump: failed to write program header table", errp); > + error_setg(errp, "dump: failed to write program header table"); > } > } > > @@ -225,7 +219,7 @@ static void write_elf64_note(DumpState *s, Error **errp) > > ret = fd_write_vmcore(&phdr, sizeof(Elf64_Phdr), s); > if (ret < 0) { > - dump_error(s, "dump: failed to write program header table", errp); > + error_setg(errp, "dump: failed to write program header table"); > } > } > > @@ -245,7 +239,7 @@ static void write_elf64_notes(WriteCoreDumpFunction f, > DumpState *s, > id = cpu_index(cpu); > ret = cpu_write_elf64_note(f, cpu, id, s); > if (ret < 0) { > - dump_error(s, "dump: failed to write elf notes", errp); > + error_setg(errp, "dump: failed to write elf notes"); > return; > } > } > @@ -253,7 +247,7 @@ static void write_elf64_notes(WriteCoreDumpFunction f, > DumpState *s, > CPU_FOREACH(cpu) { > ret = cpu_write_elf64_qemunote(f, cpu, s); > if (ret < 0) { > - dump_error(s, "dump: failed to write CPU status", errp); > + error_setg(errp, "dump: failed to write CPU status"); > return; > } > } > @@ -275,7 +269,7 @@ static void write_elf32_note(DumpState *s, Error **errp) > > ret = fd_write_vmcore(&phdr, sizeof(Elf32_Phdr), s); > if (ret < 0) { > - dump_error(s, "dump: failed to write program header table", errp); > + error_setg(errp, "dump: failed to write program header table"); > } > } > > @@ -290,7 +284,7 @@ static void write_elf32_notes(WriteCoreDumpFunction f, > DumpState *s, > id = cpu_index(cpu); > ret = cpu_write_elf32_note(f, cpu, id, s); > if (ret < 0) { > - dump_error(s, "dump: failed to write elf notes", errp); > + error_setg(errp, "dump: failed to write elf notes"); > return; > } > } > @@ -298,7 +292,7 @@ static void write_elf32_notes(WriteCoreDumpFunction f, > DumpState *s, > CPU_FOREACH(cpu) { > ret = cpu_write_elf32_qemunote(f, cpu, s); > if (ret < 0) { > - dump_error(s, "dump: failed to write CPU status", errp); > + error_setg(errp, "dump: failed to write CPU status"); > return; > } > } > @@ -326,7 +320,7 @@ static void write_elf_section(DumpState *s, int type, > Error **errp) > > ret = fd_write_vmcore(&shdr, shdr_size, s); > if (ret < 0) { > - dump_error(s, "dump: failed to write section header table", errp); > + error_setg(errp, "dump: failed to write section header table"); > } > } > > @@ -336,7 +330,7 @@ static void write_data(DumpState *s, void *buf, int > length, Error **errp) > > ret = fd_write_vmcore(buf, length, s); > if (ret < 0) { > - dump_error(s, "dump: failed to save memory", errp); > + error_setg(errp, "dump: failed to save memory"); > } > } > > @@ -568,11 +562,6 @@ static void dump_begin(DumpState *s, Error **errp) > } > } > > -static void dump_completed(DumpState *s) > -{ > - dump_cleanup(s); > -} > - > static int get_next_block(DumpState *s, GuestPhysBlock *block) > { > while (1) { > @@ -624,8 +613,6 @@ static void dump_iterate(DumpState *s, Error **errp) > } > > } while (!get_next_block(s, block)); > - > - dump_completed(s); > } > > static void create_vmcore(DumpState *s, Error **errp) > @@ -765,7 +752,7 @@ static void create_header32(DumpState *s, Error **errp) > dh->status = cpu_to_dump32(s, status); > > if (write_buffer(s->fd, 0, dh, size) < 0) { > - dump_error(s, "dump: failed to write disk dump header", errp); > + error_setg(errp, "dump: failed to write disk dump header"); > goto out; > } > > @@ -784,7 +771,7 @@ static void create_header32(DumpState *s, Error **errp) > > if (write_buffer(s->fd, DISKDUMP_HEADER_BLOCKS * > block_size, kh, size) < 0) { > - dump_error(s, "dump: failed to write kdump sub header", errp); > + error_setg(errp, "dump: failed to write kdump sub header"); > goto out; > } > > @@ -800,7 +787,7 @@ static void create_header32(DumpState *s, Error **errp) > } > if (write_buffer(s->fd, offset_note, s->note_buf, > s->note_size) < 0) { > - dump_error(s, "dump: failed to write notes", errp); > + error_setg(errp, "dump: failed to write notes"); > goto out; > } > > @@ -865,7 +852,7 @@ static void create_header64(DumpState *s, Error **errp) > dh->status = cpu_to_dump32(s, status); > > if (write_buffer(s->fd, 0, dh, size) < 0) { > - dump_error(s, "dump: failed to write disk dump header", errp); > + error_setg(errp, "dump: failed to write disk dump header"); > goto out; > } > > @@ -884,7 +871,7 @@ static void create_header64(DumpState *s, Error **errp) > > if (write_buffer(s->fd, DISKDUMP_HEADER_BLOCKS * > block_size, kh, size) < 0) { > - dump_error(s, "dump: failed to write kdump sub header", errp); > + error_setg(errp, "dump: failed to write kdump sub header"); > goto out; > } > > @@ -901,7 +888,7 @@ static void create_header64(DumpState *s, Error **errp) > > if (write_buffer(s->fd, offset_note, s->note_buf, > s->note_size) < 0) { > - dump_error(s, "dump: failed to write notes", errp); > + error_setg(errp, "dump: failed to write notes"); > goto out; > } > > @@ -1064,7 +1051,7 @@ static void write_dump_bitmap(DumpState *s, Error > **errp) > while (get_next_page(&block_iter, &pfn, NULL, s)) { > ret = set_dump_bitmap(last_pfn, pfn, true, dump_bitmap_buf, s); > if (ret < 0) { > - dump_error(s, "dump: failed to set dump_bitmap", errp); > + error_setg(errp, "dump: failed to set dump_bitmap"); > goto out; > } > > @@ -1081,7 +1068,7 @@ static void write_dump_bitmap(DumpState *s, Error > **errp) > ret = set_dump_bitmap(last_pfn, last_pfn + PFN_BUFBITMAP, false, > dump_bitmap_buf, s); > if (ret < 0) { > - dump_error(s, "dump: failed to sync dump_bitmap", errp); > + error_setg(errp, "dump: failed to sync dump_bitmap"); > goto out; > } > } > @@ -1214,7 +1201,7 @@ static void write_dump_pages(DumpState *s, Error **errp) > ret = write_cache(&page_data, buf, TARGET_PAGE_SIZE, false); > g_free(buf); > if (ret < 0) { > - dump_error(s, "dump: failed to write page data (zero page)", errp); > + error_setg(errp, "dump: failed to write page data (zero page)"); > goto out; > } > > @@ -1230,7 +1217,7 @@ static void write_dump_pages(DumpState *s, Error **errp) > ret = write_cache(&page_desc, &pd_zero, sizeof(PageDescriptor), > false); > if (ret < 0) { > - dump_error(s, "dump: failed to write page desc", errp); > + error_setg(errp, "dump: failed to write page desc"); > goto out; > } > } else { > @@ -1255,7 +1242,7 @@ static void write_dump_pages(DumpState *s, Error **errp) > > ret = write_cache(&page_data, buf_out, size_out, false); > if (ret < 0) { > - dump_error(s, "dump: failed to write page data", errp); > + error_setg(errp, "dump: failed to write page data"); > goto out; > } > #ifdef CONFIG_LZO > @@ -1268,7 +1255,7 @@ static void write_dump_pages(DumpState *s, Error **errp) > > ret = write_cache(&page_data, buf_out, size_out, false); > if (ret < 0) { > - dump_error(s, "dump: failed to write page data", errp); > + error_setg(errp, "dump: failed to write page data"); > goto out; > } > #endif > @@ -1282,7 +1269,7 @@ static void write_dump_pages(DumpState *s, Error **errp) > > ret = write_cache(&page_data, buf_out, size_out, false); > if (ret < 0) { > - dump_error(s, "dump: failed to write page data", errp); > + error_setg(errp, "dump: failed to write page data"); > goto out; > } > #endif > @@ -1297,7 +1284,7 @@ static void write_dump_pages(DumpState *s, Error **errp) > > ret = write_cache(&page_data, buf, TARGET_PAGE_SIZE, false); > if (ret < 0) { > - dump_error(s, "dump: failed to write page data", errp); > + error_setg(errp, "dump: failed to write page data"); > goto out; > } > } > @@ -1309,7 +1296,7 @@ static void write_dump_pages(DumpState *s, Error **errp) > > ret = write_cache(&page_desc, &pd, sizeof(PageDescriptor), > false); > if (ret < 0) { > - dump_error(s, "dump: failed to write page desc", errp); > + error_setg(errp, "dump: failed to write page desc"); > goto out; > } > } > @@ -1317,12 +1304,12 @@ static void write_dump_pages(DumpState *s, Error > **errp) > > ret = write_cache(&page_desc, NULL, 0, true); > if (ret < 0) { > - dump_error(s, "dump: failed to sync cache for page_desc", errp); > + error_setg(errp, "dump: failed to sync cache for page_desc"); > goto out; > } > ret = write_cache(&page_data, NULL, 0, true); > if (ret < 0) { > - dump_error(s, "dump: failed to sync cache for page_data", errp); > + error_setg(errp, "dump: failed to sync cache for page_data"); > goto out; > } > > @@ -1366,7 +1353,7 @@ static void create_kdump_vmcore(DumpState *s, Error > **errp) > > ret = write_start_flat_header(s->fd); > if (ret < 0) { > - dump_error(s, "dump: failed to write start flat header", errp); > + error_setg(errp, "dump: failed to write start flat header"); > return; > } > > @@ -1390,11 +1377,9 @@ static void create_kdump_vmcore(DumpState *s, Error > **errp) > > ret = write_end_flat_header(s->fd); > if (ret < 0) { > - dump_error(s, "dump: failed to write end flat header", errp); > + error_setg(errp, "dump: failed to write end flat header"); > return; > } > - > - dump_completed(s); > } > > static ram_addr_t get_start_block(DumpState *s) > @@ -1677,6 +1662,7 @@ void qmp_dump_guest_memory(bool paging, const char > *file, bool has_begin, > create_vmcore(s, errp); > } > > + dump_cleanup(s); > g_free(s); > } > > -- > 2.4.3 > >