From: Janosch Frank <frankja@linux.ibm.com>
To: "Marc-André Lureau" <marcandre.lureau@gmail.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
Richard Henderson <richard.henderson@linaro.org>,
QEMU <qemu-devel@nongnu.org>
Subject: Re: [PATCH v3 1/9] dump: Use ERRP_GUARD()
Date: Thu, 31 Mar 2022 11:48:33 +0200 [thread overview]
Message-ID: <750dd013-d925-fb07-e908-00ad4d7feb8a@linux.ibm.com> (raw)
In-Reply-To: <CAJ+F1CJaGg47PsiHJ2nuvZRTVqXaPeQXdFWbG4iFBeSpnV6i=A@mail.gmail.com>
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.
>
> 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
>>
>>
>>
>
next prev parent reply other threads:[~2022-03-31 9:51 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-30 12:35 [PATCH v3 0/9] dump: Cleanup and consolidation Janosch Frank
2022-03-30 12:35 ` [PATCH v3 1/9] dump: Use ERRP_GUARD() Janosch Frank
2022-03-31 8:59 ` Marc-André Lureau
2022-03-31 9:48 ` Janosch Frank [this message]
2022-03-31 10:11 ` Marc-André Lureau
2022-03-30 12:35 ` [PATCH v3 2/9] dump: Remove the sh_info variable Janosch Frank
2022-03-31 8:58 ` Marc-André Lureau
2022-03-31 9:47 ` Janosch Frank
2022-03-31 13:49 ` Marc-André Lureau
2022-04-07 9:48 ` [PATCH v4] " Janosch Frank
2022-03-30 12:35 ` [PATCH v3 3/9] dump: Introduce shdr_num to decrease complexity Janosch Frank
2022-03-31 8:56 ` Marc-André Lureau
2022-03-30 12:35 ` [PATCH v3 4/9] dump: Remove the section if when calculating the memory offset Janosch Frank
2022-03-31 8:56 ` Marc-André Lureau
2022-03-30 12:35 ` [PATCH v3 5/9] dump: Add more offset variables Janosch Frank
2022-03-30 12:36 ` [PATCH v3 6/9] dump: Introduce dump_is_64bit() helper function Janosch Frank
2022-03-31 8:56 ` Marc-André Lureau
2022-03-30 12:36 ` [PATCH v3 7/9] dump: Consolidate phdr note writes Janosch Frank
2022-03-31 8:56 ` Marc-André Lureau
2022-03-30 12:36 ` [PATCH v3 8/9] dump: Cleanup dump_begin write functions Janosch Frank
2022-03-31 8:56 ` Marc-André Lureau
2022-03-30 12:36 ` [PATCH v3 9/9] dump: Consolidate elf note function Janosch Frank
2022-03-31 8:56 ` Marc-André Lureau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=750dd013-d925-fb07-e908-00ad4d7feb8a@linux.ibm.com \
--to=frankja@linux.ibm.com \
--cc=marcandre.lureau@gmail.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=richard.henderson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.