qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 1/9] dump: Use ERRP_GUARD()

From: Janosch Frank
Subject: Re: [PATCH v3 1/9] dump: Use ERRP_GUARD()
Date: Thu, 31 Mar 2022 11:48:33 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 
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
reply via email to
[Prev in Thread] Current Thread [Next in Thread]