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.
> 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.
> }
>
> 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.
> }
> }
>
thanks
-- PMM
