From: Greg Kurz <groug@kaod.org>
To: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
qemu-ppc@nongnu.org, Markus Armbruster <armbru@redhat.com>,
David Gibson <david@gibson.dropbear.id.au>,
qemu-devel@nongnu.org
Subject: Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug()
Date: Thu, 17 Sep 2020 14:40:58 +0200 [thread overview]
Message-ID: <20200917144058.3075e5ac@bahia.lan> (raw)
In-Reply-To: <20200917121851.GA1597829@redhat.com>
On Thu, 17 Sep 2020 13:18:51 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Thu, Sep 17, 2020 at 02:04:41PM +0200, Markus Armbruster wrote:
> > Greg Kurz <groug@kaod.org> writes:
> >
> > > $ git grep object_property_get_uint -- :^{include,qom/object.c} | wc -l
> > > 60
> > >
> > > Manual inspecting the output of
> > >
> > > $ git grep -W object_property_get_uint -- :^{include,qom/object.c}
> > > ...
> > >
> > > seems to be showing that most users simply ignore errors (ie. pass NULL
> > > as 3rd argument). Then some users pass &error_abort and only a few
> > > pass a &err or errp.
> > >
> > > Assuming that users know what they're doing, I guess my proposal
> > > wouldn't bring much to the code base in the end... I'm not even
> > > sure now that it's worth changing the contract.
> >
> > We'd change
> >
> > val = object_property_get_uint(obj, name, &error_abort);
> >
> > to
> >
> > object_property_get_uint(obj, name, &val, &error_abort);
> >
> > which is not an improvement.
> >
> > Most of the ones passing NULL should probably pass &error_abort
> > instead. Doesn't change the argument.
>
> I wonder if we actually need to have an Error parameter at all for
> the getters. IIUC the only valid runtime error is probably the case
> where the property has not been set, and even then, I think properties
> usually have a default value that would be returned. All the other
> errors look like programmer errors.
>
> IOW, can we remove the Error parameter and have all the o_p_get*
> method impls use error_abort.
>
> In the rare case where a caller needs to handle a property not
> being set, they can use object_property_find() as a test before
> invoking the getter.
>
I tend to agree.
> Of course requires someone motivated to audit all the case that
> are not using NULL or error_abort and decide whether the attempt
> at passing a local errp was actually justified or not.
>
Since I've open the can of worms, I'm volunteering to do that if
we have a consensus on the fact that "the only valid runtime
error os the case the property has not been set".
Cheers,
--
Greg
> Regards,
> Daniel
next prev parent reply other threads:[~2020-09-17 12:42 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-14 12:34 [PATCH 00/15] spapr: Error handling fixes and cleanups (round 2) Greg Kurz
2020-09-14 12:34 ` [PATCH 01/15] spapr: Fix error leak in spapr_realize_vcpu() Greg Kurz
2020-09-15 9:08 ` Vladimir Sementsov-Ogievskiy
2020-09-15 13:00 ` Philippe Mathieu-Daudé
2020-09-14 12:34 ` [PATCH 02/15] ppc: Add a return value to ppc_set_compat() and ppc_set_compat_all() Greg Kurz
2020-09-15 9:18 ` Vladimir Sementsov-Ogievskiy
2020-09-15 9:34 ` Greg Kurz
2020-09-14 12:34 ` [PATCH 03/15] ppc: Fix return value in cpu_post_load() error path Greg Kurz
2020-09-15 9:50 ` Vladimir Sementsov-Ogievskiy
2020-09-15 13:01 ` Philippe Mathieu-Daudé
2020-09-14 12:34 ` [PATCH 04/15] spapr: Simplify error handling in callers of ppc_set_compat() Greg Kurz
2020-09-15 9:51 ` Vladimir Sementsov-Ogievskiy
2020-09-15 13:02 ` Philippe Mathieu-Daudé
2020-09-14 12:34 ` [PATCH 05/15] spapr: Get rid of cas_check_pvr() error reporting Greg Kurz
2020-09-15 10:03 ` Vladimir Sementsov-Ogievskiy
2020-09-15 11:00 ` [SPAM] " Greg Kurz
2020-09-14 12:34 ` [PATCH 06/15] spapr: Simplify error handling in do_client_architecture_support() Greg Kurz
2020-09-15 10:05 ` Vladimir Sementsov-Ogievskiy
2020-09-15 13:03 ` Philippe Mathieu-Daudé
2020-09-14 12:34 ` [PATCH 07/15] spapr: Simplify error handling in spapr_vio_busdev_realize() Greg Kurz
2020-09-15 10:15 ` Vladimir Sementsov-Ogievskiy
2020-09-14 12:34 ` [PATCH 08/15] spapr: Add a return value to spapr_drc_attach() Greg Kurz
2020-09-15 10:23 ` Vladimir Sementsov-Ogievskiy
2020-09-15 13:05 ` Philippe Mathieu-Daudé
2020-09-14 12:34 ` [PATCH 09/15] spapr: Simplify error handling in prop_get_fdt() Greg Kurz
2020-09-15 10:26 ` Vladimir Sementsov-Ogievskiy
2020-09-14 12:35 ` [PATCH 10/15] spapr: Add a return value to spapr_set_vcpu_id() Greg Kurz
2020-09-15 10:32 ` Vladimir Sementsov-Ogievskiy
2020-09-15 13:08 ` Philippe Mathieu-Daudé
2020-09-15 13:53 ` Greg Kurz
2020-09-17 1:06 ` David Gibson
2020-09-14 12:35 ` [PATCH 11/15] spapr: Simplify error handling in spapr_cpu_core_realize() Greg Kurz
2020-09-15 10:38 ` Vladimir Sementsov-Ogievskiy
2020-09-15 13:08 ` Philippe Mathieu-Daudé
2020-09-14 12:35 ` [PATCH 12/15] spapr: Add a return value to spapr_nvdimm_validate() Greg Kurz
2020-09-15 10:49 ` Vladimir Sementsov-Ogievskiy
2020-09-15 13:09 ` Philippe Mathieu-Daudé
2020-09-14 12:35 ` [PATCH 13/15] spapr: Add a return value to spapr_check_pagesize() Greg Kurz
2020-09-15 10:52 ` Vladimir Sementsov-Ogievskiy
2020-09-15 13:10 ` Philippe Mathieu-Daudé
2020-09-14 12:35 ` [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug() Greg Kurz
2020-09-15 10:58 ` Vladimir Sementsov-Ogievskiy
2020-09-15 11:43 ` [SPAM] " Greg Kurz
2020-09-15 11:53 ` Vladimir Sementsov-Ogievskiy
2020-09-15 12:04 ` Greg Kurz
2020-09-15 13:43 ` Greg Kurz
2020-09-17 1:15 ` [SPAM] " David Gibson
2020-09-17 7:38 ` Markus Armbruster
2020-09-17 10:04 ` Greg Kurz
2020-09-17 12:04 ` Markus Armbruster
2020-09-17 12:18 ` Daniel P. Berrangé
2020-09-17 12:40 ` Greg Kurz [this message]
2020-09-17 13:17 ` Markus Armbruster
2020-09-14 12:35 ` [PATCH 15/15] spapr: Simplify error handling in spapr_memory_unplug_request() Greg Kurz
2020-09-16 2:49 ` [PATCH 00/15] spapr: Error handling fixes and cleanups (round 2) David Gibson
2020-09-17 1:08 ` David Gibson
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=20200917144058.3075e5ac@bahia.lan \
--to=groug@kaod.org \
--cc=armbru@redhat.com \
--cc=berrange@redhat.com \
--cc=david@gibson.dropbear.id.au \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
--cc=vsementsov@virtuozzo.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).