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.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