Re: [PATCH v2 04/13] contrib/elf2dmp: Conform to the error reporting pattern

[Akihiko Odaki]

On 2024/03/05 22:28, Peter Maydell wrote:

On Tue, 5 Mar 2024 at 07:36, Akihiko Odaki <akihiko.od...@daynix.com> wrote:

include/qapi/error.h says:

We recommend
* bool-valued functions return true on success / false on failure,
...

Signed-off-by: Akihiko Odaki <akihiko.od...@daynix.com>
---
  contrib/elf2dmp/addrspace.h |   6 +--
  contrib/elf2dmp/download.h  |   2 +-
  contrib/elf2dmp/pdb.h       |   2 +-
  contrib/elf2dmp/qemu_elf.h  |   2 +-
  contrib/elf2dmp/addrspace.c |  12 ++---
  contrib/elf2dmp/download.c  |  10 ++--
  contrib/elf2dmp/main.c      | 114 +++++++++++++++++++++-----------------------
  contrib/elf2dmp/pdb.c       |  50 +++++++++----------
  contrib/elf2dmp/qemu_elf.c  |  32 ++++++-------
  9 files changed, 112 insertions(+), 118 deletions(-)

This is a bit big to review easily. Converting one function
(or a small set of closely related functions) at once would
make for an easier to review split.

I'll split patches for each source files.

diff --git a/contrib/elf2dmp/download.c b/contrib/elf2dmp/download.c
index 902dc04ffa5c..ec8d33ba1e4b 100644
--- a/contrib/elf2dmp/download.c
+++ b/contrib/elf2dmp/download.c
@@ -9,14 +9,14 @@
  #include <curl/curl.h>
  #include "download.h"

-int download_url(const char *name, const char *url)
+bool download_url(const char *name, const char *url)
  {
-    int err = 1;
+    bool success = false;
      FILE *file;
      CURL *curl = curl_easy_init();

      if (!curl) {
-        return 1;
+        return success;

Why not just "return false" ? "return success" makes it look
like a success-path, not a failure-path.

It is a mistake. I'll fix in the next version.

      }

      file = fopen(name, "wb");
@@ -33,11 +33,11 @@ int download_url(const char *name, const char *url)
          unlink(name);
          fclose(file);
      } else {
-        err = fclose(file);
+        success = !fclose(file);
      }

  out_curl:
      curl_easy_cleanup(curl);

-    return err;
+    return success;
  }

@@ -186,13 +186,13 @@ static void 
win_context_init_from_qemu_cpu_state(WinContext64 *ctx,
   * Finds paging-structure hierarchy base,
   * if previously set doesn't give access to kernel structures
   */
-static int fix_dtb(struct va_space *vs, QEMU_Elf *qe)
+static bool fix_dtb(struct va_space *vs, QEMU_Elf *qe)
  {
      /*
       * Firstly, test previously set DTB.
       */
      if (va_space_resolve(vs, SharedUserData)) {
-        return 0;
+        return true;
      }

      /*
@@ -206,7 +206,7 @@ static int fix_dtb(struct va_space *vs, QEMU_Elf *qe)
              va_space_set_dtb(vs, s->cr[3]);
              printf("DTB 0x%016"PRIx64" has been found from CPU #%zu"
                      " as system task CR3\n", vs->dtb, i);
-            return !(va_space_resolve(vs, SharedUserData));
+            return !!(va_space_resolve(vs, SharedUserData));

If the function returns bool type, we don't need the !! idiom
to coerce the value to bool.

va_space_resolve() returns void *.

Regards,
Akihiko Odaki