On Thu, Mar 31, 2022 at 1:48 PM Janosch Frank <fran...@linux.ibm.com> wrote:
> On 3/31/22 10:59, Marc-André Lureau wrote:
> > Hi
> >
> > On Wed, Mar 30, 2022 at 4:42 PM Janosch Frank <fran...@linux.ibm.com>
> wrote:
> >
> >> Let's move to the new way of handling errors before changing the dump
> >> code. This patch has mostly been generated by the coccinelle script
> >> scripts/coccinelle/errp-guard.cocci.
> >>
> >> Signed-off-by: Janosch Frank <fran...@linux.ibm.com>
> >> Reviewed-by: Richard Henderson <richard.hender...@linaro.org>
> >>
> >
> > nice
> > Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>
> Thanks!
>
> >
> > While you are at it, would you add a patch (at the end of this series, to
> > avoid the churn) to return a bool for success/failure (as recommended in
> > qapi/error.h)?
>
> I knew it was a mistake to touch this file :)
>
> Sure, it will be churn anyway since I have two more patch sets that
> follow on this one.
>
>
Feel free to leave that cleanup for now
thanks
> >
> > thanks
> >
> >
> >> ---
> >> dump/dump.c | 144 ++++++++++++++++++++++------------------------------
> >> 1 file changed, 61 insertions(+), 83 deletions(-)
> >>
> >> diff --git a/dump/dump.c b/dump/dump.c
> >> index f57ed76fa7..58c4923fce 100644
> >> --- a/dump/dump.c
> >> +++ b/dump/dump.c
> >> @@ -390,23 +390,21 @@ static void write_data(DumpState *s, void *buf,
> int
> >> length, Error **errp)
> >> static void write_memory(DumpState *s, GuestPhysBlock *block,
> ram_addr_t
> >> start,
> >> int64_t size, Error **errp)
> >> {
> >> + ERRP_GUARD();
> >> int64_t i;
> >> - Error *local_err = NULL;
> >>
> >> for (i = 0; i < size / s->dump_info.page_size; i++) {
> >> write_data(s, block->host_addr + start + i *
> >> s->dump_info.page_size,
> >> - s->dump_info.page_size, &local_err);
> >> - if (local_err) {
> >> - error_propagate(errp, local_err);
> >> + s->dump_info.page_size, errp);
> >> + if (*errp) {
> >> return;
> >> }
> >> }
> >>
> >> if ((size % s->dump_info.page_size) != 0) {
> >> write_data(s, block->host_addr + start + i *
> >> s->dump_info.page_size,
> >> - size % s->dump_info.page_size, &local_err);
> >> - if (local_err) {
> >> - error_propagate(errp, local_err);
> >> + size % s->dump_info.page_size, errp);
> >> + if (*errp) {
> >> return;
> >> }
> >> }
> >> @@ -476,11 +474,11 @@ static void get_offset_range(hwaddr phys_addr,
> >>
> >> static void write_elf_loads(DumpState *s, Error **errp)
> >> {
> >> + ERRP_GUARD();
> >> hwaddr offset, filesz;
> >> MemoryMapping *memory_mapping;
> >> uint32_t phdr_index = 1;
> >> uint32_t max_index;
> >> - Error *local_err = NULL;
> >>
> >> if (s->have_section) {
> >> max_index = s->sh_info;
> >> @@ -494,14 +492,13 @@ static void write_elf_loads(DumpState *s, Error
> >> **errp)
> >> s, &offset, &filesz);
> >> if (s->dump_info.d_class == ELFCLASS64) {
> >> write_elf64_load(s, memory_mapping, phdr_index++, offset,
> >> - filesz, &local_err);
> >> + filesz, errp);
> >> } else {
> >> write_elf32_load(s, memory_mapping, phdr_index++, offset,
> >> - filesz, &local_err);
> >> + filesz, errp);
> >> }
> >>
> >> - if (local_err) {
> >> - error_propagate(errp, local_err);
> >> + if (*errp) {
> >> return;
> >> }
> >>
> >> @@ -514,7 +511,7 @@ static void write_elf_loads(DumpState *s, Error
> **errp)
> >> /* write elf header, PT_NOTE and elf note to vmcore. */
> >> static void dump_begin(DumpState *s, Error **errp)
> >> {
> >> - Error *local_err = NULL;
> >> + ERRP_GUARD();
> >>
> >> /*
> >> * the vmcore's format is:
> >> @@ -542,73 +539,64 @@ static void dump_begin(DumpState *s, Error **errp)
> >>
> >> /* write elf header to vmcore */
> >> if (s->dump_info.d_class == ELFCLASS64) {
> >> - write_elf64_header(s, &local_err);
> >> + write_elf64_header(s, errp);
> >> } else {
> >> - write_elf32_header(s, &local_err);
> >> + write_elf32_header(s, errp);
> >> }
> >> - if (local_err) {
> >> - error_propagate(errp, local_err);
> >> + if (*errp) {
> >> return;
> >> }
> >>
> >> if (s->dump_info.d_class == ELFCLASS64) {
> >> /* write PT_NOTE to vmcore */
> >> - write_elf64_note(s, &local_err);
> >> - if (local_err) {
> >> - error_propagate(errp, local_err);
> >> + write_elf64_note(s, errp);
> >> + if (*errp) {
> >> return;
> >> }
> >>
> >> /* write all PT_LOAD to vmcore */
> >> - write_elf_loads(s, &local_err);
> >> - if (local_err) {
> >> - error_propagate(errp, local_err);
> >> + write_elf_loads(s, errp);
> >> + if (*errp) {
> >> return;
> >> }
> >>
> >> /* write section to vmcore */
> >> if (s->have_section) {
> >> - write_elf_section(s, 1, &local_err);
> >> - if (local_err) {
> >> - error_propagate(errp, local_err);
> >> + write_elf_section(s, 1, errp);
> >> + if (*errp) {
> >> return;
> >> }
> >> }
> >>
> >> /* write notes to vmcore */
> >> - write_elf64_notes(fd_write_vmcore, s, &local_err);
> >> - if (local_err) {
> >> - error_propagate(errp, local_err);
> >> + write_elf64_notes(fd_write_vmcore, s, errp);
> >> + if (*errp) {
> >> return;
> >> }
> >> } else {
> >> /* write PT_NOTE to vmcore */
> >> - write_elf32_note(s, &local_err);
> >> - if (local_err) {
> >> - error_propagate(errp, local_err);
> >> + write_elf32_note(s, errp);
> >> + if (*errp) {
> >> return;
> >> }
> >>
> >> /* write all PT_LOAD to vmcore */
> >> - write_elf_loads(s, &local_err);
> >> - if (local_err) {
> >> - error_propagate(errp, local_err);
> >> + write_elf_loads(s, errp);
> >> + if (*errp) {
> >> return;
> >> }
> >>
> >> /* write section to vmcore */
> >> if (s->have_section) {
> >> - write_elf_section(s, 0, &local_err);
> >> - if (local_err) {
> >> - error_propagate(errp, local_err);
> >> + write_elf_section(s, 0, errp);
> >> + if (*errp) {
> >> return;
> >> }
> >> }
> >>
> >> /* write notes to vmcore */
> >> - write_elf32_notes(fd_write_vmcore, s, &local_err);
> >> - if (local_err) {
> >> - error_propagate(errp, local_err);
> >> + write_elf32_notes(fd_write_vmcore, s, errp);
> >> + if (*errp) {
> >> return;
> >> }
> >> }
> >> @@ -644,9 +632,9 @@ static int get_next_block(DumpState *s,
> GuestPhysBlock
> >> *block)
> >> /* write all memory to vmcore */
> >> static void dump_iterate(DumpState *s, Error **errp)
> >> {
> >> + ERRP_GUARD();
> >> GuestPhysBlock *block;
> >> int64_t size;
> >> - Error *local_err = NULL;
> >>
> >> do {
> >> block = s->next_block;
> >> @@ -658,9 +646,8 @@ static void dump_iterate(DumpState *s, Error **errp)
> >> size -= block->target_end - (s->begin + s->length);
> >> }
> >> }
> >> - write_memory(s, block, s->start, size, &local_err);
> >> - if (local_err) {
> >> - error_propagate(errp, local_err);
> >> + write_memory(s, block, s->start, size, errp);
> >> + if (*errp) {
> >> return;
> >> }
> >>
> >> @@ -669,11 +656,10 @@ static void dump_iterate(DumpState *s, Error
> **errp)
> >>
> >> static void create_vmcore(DumpState *s, Error **errp)
> >> {
> >> - Error *local_err = NULL;
> >> + ERRP_GUARD();
> >>
> >> - dump_begin(s, &local_err);
> >> - if (local_err) {
> >> - error_propagate(errp, local_err);
> >> + dump_begin(s, errp);
> >> + if (*errp) {
> >> return;
> >> }
> >>
> >> @@ -810,6 +796,7 @@ static bool note_name_equal(DumpState *s,
> >> /* write common header, sub header and elf note to vmcore */
> >> static void create_header32(DumpState *s, Error **errp)
> >> {
> >> + ERRP_GUARD();
> >> DiskDumpHeader32 *dh = NULL;
> >> KdumpSubHeader32 *kh = NULL;
> >> size_t size;
> >> @@ -818,7 +805,6 @@ static void create_header32(DumpState *s, Error
> **errp)
> >> uint32_t bitmap_blocks;
> >> uint32_t status = 0;
> >> uint64_t offset_note;
> >> - Error *local_err = NULL;
> >>
> >> /* write common header, the version of kdump-compressed format is
> 6th
> >> */
> >> size = sizeof(DiskDumpHeader32);
> >> @@ -894,9 +880,8 @@ static void create_header32(DumpState *s, Error
> **errp)
> >> s->note_buf_offset = 0;
> >>
> >> /* use s->note_buf to store notes temporarily */
> >> - write_elf32_notes(buf_write_note, s, &local_err);
> >> - if (local_err) {
> >> - error_propagate(errp, local_err);
> >> + write_elf32_notes(buf_write_note, s, errp);
> >> + if (*errp) {
> >> goto out;
> >> }
> >> if (write_buffer(s->fd, offset_note, s->note_buf,
> >> @@ -922,6 +907,7 @@ out:
> >> /* write common header, sub header and elf note to vmcore */
> >> static void create_header64(DumpState *s, Error **errp)
> >> {
> >> + ERRP_GUARD();
> >> DiskDumpHeader64 *dh = NULL;
> >> KdumpSubHeader64 *kh = NULL;
> >> size_t size;
> >> @@ -930,7 +916,6 @@ static void create_header64(DumpState *s, Error
> **errp)
> >> uint32_t bitmap_blocks;
> >> uint32_t status = 0;
> >> uint64_t offset_note;
> >> - Error *local_err = NULL;
> >>
> >> /* write common header, the version of kdump-compressed format is
> 6th
> >> */
> >> size = sizeof(DiskDumpHeader64);
> >> @@ -1006,9 +991,8 @@ static void create_header64(DumpState *s, Error
> >> **errp)
> >> s->note_buf_offset = 0;
> >>
> >> /* use s->note_buf to store notes temporarily */
> >> - write_elf64_notes(buf_write_note, s, &local_err);
> >> - if (local_err) {
> >> - error_propagate(errp, local_err);
> >> + write_elf64_notes(buf_write_note, s, errp);
> >> + if (*errp) {
> >> goto out;
> >> }
> >>
> >> @@ -1464,8 +1448,8 @@ out:
> >>
> >> static void create_kdump_vmcore(DumpState *s, Error **errp)
> >> {
> >> + ERRP_GUARD();
> >> int ret;
> >> - Error *local_err = NULL;
> >>
> >> /*
> >> * the kdump-compressed format is:
> >> @@ -1495,21 +1479,18 @@ static void create_kdump_vmcore(DumpState *s,
> >> Error **errp)
> >> return;
> >> }
> >>
> >> - write_dump_header(s, &local_err);
> >> - if (local_err) {
> >> - error_propagate(errp, local_err);
> >> + write_dump_header(s, errp);
> >> + if (*errp) {
> >> return;
> >> }
> >>
> >> - write_dump_bitmap(s, &local_err);
> >> - if (local_err) {
> >> - error_propagate(errp, local_err);
> >> + write_dump_bitmap(s, errp);
> >> + if (*errp) {
> >> return;
> >> }
> >>
> >> - write_dump_pages(s, &local_err);
> >> - if (local_err) {
> >> - error_propagate(errp, local_err);
> >> + write_dump_pages(s, errp);
> >> + if (*errp) {
> >> return;
> >> }
> >>
> >> @@ -1639,10 +1620,10 @@ static void dump_init(DumpState *s, int fd, bool
> >> has_format,
> >> DumpGuestMemoryFormat format, bool paging, bool
> >> has_filter,
> >> int64_t begin, int64_t length, Error **errp)
> >> {
> >> + ERRP_GUARD();
> >> VMCoreInfoState *vmci = vmcoreinfo_find();
> >> CPUState *cpu;
> >> int nr_cpus;
> >> - Error *err = NULL;
> >> int ret;
> >>
> >> s->has_format = has_format;
> >> @@ -1761,9 +1742,8 @@ static void dump_init(DumpState *s, int fd, bool
> >> has_format,
> >>
> >> /* get memory mapping */
> >> if (paging) {
> >> - qemu_get_guest_memory_mapping(&s->list, &s->guest_phys_blocks,
> >> &err);
> >> - if (err != NULL) {
> >> - error_propagate(errp, err);
> >> + qemu_get_guest_memory_mapping(&s->list, &s->guest_phys_blocks,
> >> errp);
> >> + if (*errp) {
> >> goto cleanup;
> >> }
> >> } else {
> >> @@ -1862,33 +1842,32 @@ cleanup:
> >> /* this operation might be time consuming. */
> >> static void dump_process(DumpState *s, Error **errp)
> >> {
> >> - Error *local_err = NULL;
> >> + ERRP_GUARD();
> >> DumpQueryResult *result = NULL;
> >>
> >> if (s->has_format && s->format ==
> DUMP_GUEST_MEMORY_FORMAT_WIN_DMP) {
> >> #ifdef TARGET_X86_64
> >> - create_win_dump(s, &local_err);
> >> + create_win_dump(s, errp);
> >> #endif
> >> } else if (s->has_format && s->format !=
> >> DUMP_GUEST_MEMORY_FORMAT_ELF) {
> >> - create_kdump_vmcore(s, &local_err);
> >> + create_kdump_vmcore(s, errp);
> >> } else {
> >> - create_vmcore(s, &local_err);
> >> + create_vmcore(s, errp);
> >> }
> >>
> >> /* make sure status is written after written_size updates */
> >> smp_wmb();
> >> qatomic_set(&s->status,
> >> - (local_err ? DUMP_STATUS_FAILED :
> DUMP_STATUS_COMPLETED));
> >> + (*errp ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED));
> >>
> >> /* send DUMP_COMPLETED message (unconditionally) */
> >> result = qmp_query_dump(NULL);
> >> /* should never fail */
> >> assert(result);
> >> - qapi_event_send_dump_completed(result, !!local_err, (local_err ?
> >> - error_get_pretty(local_err) :
> NULL));
> >> + qapi_event_send_dump_completed(result, !!*errp, (*errp ?
> >> +
> >> error_get_pretty(*errp) : NULL));
> >> qapi_free_DumpQueryResult(result);
> >>
> >> - error_propagate(errp, local_err);
> >> dump_cleanup(s);
> >> }
> >>
> >> @@ -1917,10 +1896,10 @@ void qmp_dump_guest_memory(bool paging, const
> char
> >> *file,
> >> int64_t length, bool has_format,
> >> DumpGuestMemoryFormat format, Error **errp)
> >> {
> >> + ERRP_GUARD();
> >> const char *p;
> >> int fd = -1;
> >> DumpState *s;
> >> - Error *local_err = NULL;
> >> bool detach_p = false;
> >>
> >> if (runstate_check(RUN_STATE_INMIGRATE)) {
> >> @@ -2020,9 +1999,8 @@ void qmp_dump_guest_memory(bool paging, const char
> >> *file,
> >> dump_state_prepare(s);
> >>
> >> dump_init(s, fd, has_format, format, paging, has_begin,
> >> - begin, length, &local_err);
> >> - if (local_err) {
> >> - error_propagate(errp, local_err);
> >> + begin, length, errp);
> >> + if (*errp) {
> >> qatomic_set(&s->status, DUMP_STATUS_FAILED);
> >> return;
> >> }
> >> --
> >> 2.32.0
> >>
> >>
> >>
> >
>
>
--
Marc-André Lureau
