[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 for-5.2 1/2] spapr: Use error_append_hint() in spapr_caps.
From: |
David Gibson |
Subject: |
Re: [PATCH v4 for-5.2 1/2] spapr: Use error_append_hint() in spapr_caps.c |
Date: |
Tue, 28 Jul 2020 20:09:37 +1000 |
On Tue, Jul 28, 2020 at 11:07:26AM +0200, Greg Kurz wrote:
> On Tue, 28 Jul 2020 09:26:08 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
> > Greg Kurz <groug@kaod.org> writes:
> >
> > > On Mon, 20 Jul 2020 17:24:35 +0200
> > > Markus Armbruster <armbru@redhat.com> wrote:
> > >
> > >> Greg Kurz <groug@kaod.org> writes:
> > >>
> > >> > We have a dedicated error API for hints. Use it instead of embedding
> > >> > the hint in the error message, as recommanded in the "qapi/error.h"
> > >> > header file.
> > >> >
> > >> > Since spapr_caps_apply() passes &error_fatal, all functions must
> > >> > also call the ERRP_GUARD() macro for error_append_hint() to be
> > >> > functional.
> > >>
> > >> This isn't a request for change in this patch, just an attempt to squash
> > >> possible misunderstandings.
> > >>
> > >> It's true that error_append_hint() without ERRP_GUARD() works as long as
> > >> the caller doesn't pass certain errp arguments. But the callee should
> > >> work for all possible @errp arguments, not just the ones that get passed
> > >> today. That's why error.h wants you to guard *all* uses of
> > >> error_append_hint(errp):
> > >>
> > >> * = Why, when and how to use ERRP_GUARD() =
> > >> *
> > >> * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> > >> * - It must not be dereferenced, because it may be null.
> > >> * - It should not be passed to error_prepend() or
> > >> * error_append_hint(), because that doesn't work with &error_fatal.
> > >> * ERRP_GUARD() lifts these restrictions.
> > >>
> > >
> > > Yeah, I just wanted to emphasize that we were precisely in the case
> > > where we _really_ need to lift the restriction, but I'm perfectly fine
> > > with dropping this sentence if you consider it useless.
> >
> > I lean towards dropping it.
> >
>
> David,
>
> Do you want me to send an updated version of this patch or can you
> fix this in your tree ?
Updated version please, I'm afraid I've lost track of quite what was
suggested in this thread.
>
> > > BTW, should we have a way for CI to ensure that a patch that adds
> > > error_prepend(errp, ...) or error_append_hint(errp, ...) also adds
> > > ERRP_GUARD() ? Not sure that people read error.h that often...
> >
> > I don't know. Wait and see whether it's worth automating? We didn't
> > automate checking other Error API rules, like "no newlines in error
> > messages". That one can't crash, though.
> >
> > The check would have to look beyond the patch, which checkpatch.pl
> > doesn't do.
> >
>
> <thinking aloud>
> Maybe checkpatch.pl could be fed with an extended version of the patch
> that has enough context, eg. git show -U$(wc -l ${file}) ${file} ?
> </thinking aloud>
>
> > >> No need to make an argument involving the possible arguments (pardon the
> > >> pun).
> > >>
> > >
> > > :)
> > >
> > >> [...]
> > >>
> >
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
[PATCH v4 for-5.2 2/2] spapr: Forbid nested KVM-HV in pre-power9 compat mode, Greg Kurz, 2020/07/16