Re: [PATCH 2/3] hw/display: Allow vga_common_init() to return errors

[Markus Armbruster <armbru@redhat.com>]

Thomas Huth <thuth@redhat.com> writes:

> On 16/03/2022 14.32, Philippe Mathieu-Daudé wrote:
>> On 16/3/22 14:24, Thomas Huth wrote:
>>> The vga_common_init() function currently cannot report errors to its
>>> caller. But in the following patch, we'd need this possibility, so
>>> let's change it to take an "Error **" as parameter for this.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   hw/display/ati.c            |  7 ++++++-
>>>   hw/display/cirrus_vga.c     |  7 ++++++-
>>>   hw/display/cirrus_vga_isa.c |  7 ++++++-
>>>   hw/display/qxl.c            |  6 +++++-
>>>   hw/display/vga-isa.c        |  9 ++++++++-
>>>   hw/display/vga-mmio.c       |  8 +++++++-
>>>   hw/display/vga-pci.c        | 15 +++++++++++++--
>>>   hw/display/vga.c            |  9 +++++++--
>>>   hw/display/vga_int.h        |  2 +-
>>>   hw/display/virtio-vga.c     |  7 ++++++-
>>>   hw/display/vmware_vga.c     |  2 +-
>>>   11 files changed, 66 insertions(+), 13 deletions(-)
>>
>> Please setup scripts/git.orderfile :)
>> 
>>> diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
>>> index 847e784ca6..3e8892df28 100644
>>> --- a/hw/display/vga_int.h
>>> +++ b/hw/display/vga_int.h
>>> @@ -156,7 +156,7 @@ static inline int c6_to_8(int v)
>>>       return (v << 2) | (b << 1) | b;
>>>   }
>>> -void vga_common_init(VGACommonState *s, Object *obj);
>>> +void vga_common_init(VGACommonState *s, Object *obj, Error **errp);
>> 
>> Can we also return a boolean value? IIUC Markus recommended to check
>> a boolean return value rather than Error* handle.
>
> Really? A very quick grep shows something different:
>
> $ grep -r ^void.*Error include/ | wc -l
> 94
> $ grep -r ^bool.*Error include/ | wc -l
> 46

Historical reasons.  We deviated from GLib here only to find out that
the deviation leads to awkward code.  I flipped the guidance in commit
e3fe3988d7 "error: Document Error API usage rules" (2020-07-10).  A lot
of old code remains.

> I also can't see that recommendation in docs/devel/style.rst. I think you 
> either got that wrong, or the coding style needs an update first.

It's in include/qapi/error.h:

/*
 * Error reporting system loosely patterned after Glib's GError.
 *
 * = Rules =

[...]

 * - Whenever practical, also return a value that indicates success /
 *   failure.  This can make the error checking more concise, and can
 *   avoid useless error object creation and destruction.  Note that
 *   we still have many functions returning void.  We recommend
 *   • bool-valued functions return true on success / false on failure,
 *   • pointer-valued functions return non-null / null pointer, and
 *   • integer-valued functions return non-negative / negative.

[...]

 * Call a function, receive an error from it, and pass it to the caller
 * - when the function returns a value that indicates failure, say
 *   false:
 *     if (!foo(arg, errp)) {
 *         handle the error...
 *     }
 * - when it does not, say because it is a void function:
 *     ERRP_GUARD();
 *     foo(arg, errp);
 *     if (*errp) {
 *         handle the error...
 *     }
 * More on ERRP_GUARD() below.

[...]

 * Receive an error, and handle it locally
 * - when the function returns a value that indicates failure, say
 *   false:
 *     Error *err = NULL;
 *     if (!foo(arg, &err)) {
 *         handle the error...
 *     }
 * - when it does not, say because it is a void function:
 *     Error *err = NULL;
 *     foo(arg, &err);
 *     if (err) {
 *         handle the error...
 *     }

Note that error.h's big comment I abbreviated here has some 200
non-blank lines.  It's too long and detailed for style.rst (which has
some 500 non-blank lines).  Instead style.rst points to error.h under
QEMU Specific Idioms / Error handling and reporting / Propagating
errors.