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

[Peter Maydell <peter.maydell@linaro.org>]

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.


> 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