On 3/31/22 10:59, Marc-André Lureau wrote:
> Hi
>
> On Wed, Mar 30, 2022 at 4:42 PM Janosch Frank <frankja@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 <frankja@linux.ibm.com>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>
>
> nice
> Reviewed-by: Marc-André Lureau <marcandre.lureau@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
>>
>>
>>
>