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>
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)?
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
