|
From: | Akihiko Odaki |
Subject: | Re: [PATCH v2 04/13] contrib/elf2dmp: Conform to the error reporting pattern |
Date: | Wed, 6 Mar 2024 14:00:58 +0900 |
User-agent: | Mozilla Thunderbird |
On 2024/03/05 22:28, Peter Maydell wrote:
On Tue, 5 Mar 2024 at 07:36, Akihiko Odaki <akihiko.odaki@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.odaki@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
[Prev in Thread] | Current Thread | [Next in Thread] |