From: Markus Armbruster <armbru@redhat.com>
To: Thomas Huth <thuth@redhat.com>
Cc: "Philippe Mathieu-Daudé" <philippe.mathieu.daude@gmail.com>,
qemu-devel@nongnu.org, "Gerd Hoffmann" <kraxel@redhat.com>
Subject: Re: [PATCH 2/3] hw/display: Allow vga_common_init() to return errors
Date: Wed, 16 Mar 2022 15:16:38 +0100 [thread overview]
Message-ID: <87bky6ugmx.fsf@pond.sub.org> (raw)
In-Reply-To: <e5e03938-d602-c928-07ef-da6486fb99b0@redhat.com> (Thomas Huth's message of "Wed, 16 Mar 2022 14:39:54 +0100")
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.
next prev parent reply other threads:[~2022-03-16 14:22 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-16 13:23 [PATCH 0/3] Fix crash when adding a second ISA VGA device Thomas Huth
2022-03-16 13:24 ` [PATCH 1/3] hw/display/cirrus_vga: Clean up indentation in pci_cirrus_vga_realize() Thomas Huth
2022-03-16 13:33 ` Philippe Mathieu-Daudé
2022-03-16 13:24 ` [PATCH 2/3] hw/display: Allow vga_common_init() to return errors Thomas Huth
2022-03-16 13:32 ` Philippe Mathieu-Daudé
2022-03-16 13:39 ` Thomas Huth
2022-03-16 14:16 ` Markus Armbruster [this message]
2022-03-16 17:05 ` Thomas Huth
2022-03-17 7:40 ` Markus Armbruster
2022-03-17 12:32 ` Thomas Huth
2022-03-17 13:29 ` Markus Armbruster
2022-03-16 13:24 ` [PATCH 3/3] hw/display/vga: Report a proper error when adding a 2nd ISA VGA Thomas Huth
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87bky6ugmx.fsf@pond.sub.org \
--to=armbru@redhat.com \
--cc=kraxel@redhat.com \
--cc=philippe.mathieu.daude@gmail.com \
--cc=qemu-devel@nongnu.org \
--cc=thuth@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).