Thomas Huth <th...@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 <th...@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.
