From: Greg Kurz <groug@kaod.org>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Cc: David Gibson <david@gibson.dropbear.id.au>,
qemu-ppc@nongnu.org, qemu-devel@nongnu.org,
Markus Armbruster <armbru@redhat.com>
Subject: Re: [PATCH 14/15] spapr: Simplify error handling in spapr_memory_plug()
Date: Tue, 15 Sep 2020 14:04:23 +0200 [thread overview]
Message-ID: <20200915140423.7bb9dfdb@bahia.lan> (raw)
In-Reply-To: <91774a03-3c70-4eab-8e02-137c6c151cb5@virtuozzo.com>
I've dropped the SPAM mentions from the subject this time :)
On Tue, 15 Sep 2020 14:53:53 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 15.09.2020 14:43, Greg Kurz wrote:
> > On Tue, 15 Sep 2020 13:58:53 +0300
> > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> >
> >> 14.09.2020 15:35, Greg Kurz wrote:
> >>> As recommended in "qapi/error.h", add a bool return value to
> >>> spapr_add_lmbs() and spapr_add_nvdimm(), and use them instead
> >>> of local_err in spapr_memory_plug().
> >>>
> >>> Since object_property_get_uint() only returns 0 on failure, use
> >>> that as well.
> >>
> >> Why are you sure? Can't the property be 0 itself?
> >>
> >
> > Hmmm... I've based this assumption on the header:
> >
> > * Returns: the value of the property, converted to an unsigned integer, or 0
> > * an error occurs (including when the property value is not an integer).
> >
> > but looking at the implementation, I don't see any check that
> > a property cannot be 0 indeed...
> >
> > It's a bit misleading to mention this in the header though. I
> > understand that the function should return something, which
> > should have a some explicitly assigned value to avoid compilers
> > or static analyzers to yell, but the written contract should be
> > that the value is _undefined_ IMHO.
> >
> > In its present form, the only way to know if the property is
> > valid is to pass a non-NULL errp actually. I'd rather even see
> > that in the contract, and an assert() in the code.
> >
> > An alternative would be to convert it to have a bool return
> > value and get the actual uint result with a pointer argument.
> >
> >>>
> >>> Also call ERRP_GUARD() to be able to check the status of void
> >>> function pc_dimm_plug() with *errp.
> >>>
> >>
> >>
> >
> > I'm now hesitating to either check *errp for object_property_get_uint()
> > too or simply drop this patch...
> >
>
> .. and the following. If no more reviewers come to look at it, you can just merge first 13 patches, not resending the whole series for last two patches, which may be moved to round 3.
>
I don't expect much people except David or maybe Markus to look
at these patches actually. And anyway, it's up to David to merge
them. But, yes, I agree patch 14 and 15 should go to the next
round.
Thanks for the review !
Cheers,
--
Greg
next prev parent reply other threads:[~2020-09-15 12:08 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 [this message]
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
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=20200915140423.7bb9dfdb@bahia.lan \
--to=groug@kaod.org \
--cc=armbru@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).