[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v12 1/8] error: New macro ERRP_AUTO_PROPAGATE()
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v12 1/8] error: New macro ERRP_AUTO_PROPAGATE() |
Date: |
Tue, 07 Jul 2020 22:43:29 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 07.07.2020 19:50, Markus Armbruster wrote:
>> From: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>>
>> Introduce a new ERRP_AUTO_PROPAGATE macro, to be used at start of
>> functions with an errp OUT parameter.
>>
>> It has three goals:
>>
>> 1. Fix issue with error_fatal and error_prepend/error_append_hint: user
>> can't see this additional information, because exit() happens in
>> error_setg earlier than information is added. [Reported by Greg Kurz]
>>
>> 2. Fix issue with error_abort and error_propagate: when we wrap
>> error_abort by local_err+error_propagate, the resulting coredump will
>> refer to error_propagate and not to the place where error happened.
>> (the macro itself doesn't fix the issue, but it allows us to [3.] drop
>> the local_err+error_propagate pattern, which will definitely fix the
>> issue) [Reported by Kevin Wolf]
>>
>> 3. Drop local_err+error_propagate pattern, which is used to workaround
>> void functions with errp parameter, when caller wants to know resulting
>> status. (Note: actually these functions could be merely updated to
>> return int error code).
>>
>> To achieve these goals, later patches will add invocations
>> of this macro at the start of functions with either use
>> error_prepend/error_append_hint (solving 1) or which use
>> local_err+error_propagate to check errors, switching those
>> functions to use *errp instead (solving 2 and 3).
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
>> Reviewed-by: Paul Durrant<paul@xen.org>
>> Reviewed-by: Greg Kurz<groug@kaod.org>
>> Reviewed-by: Eric Blake<eblake@redhat.com>
>> [Comments merged properly with recent commit "error: Document Error
>> API usage rules", and edited for clarity. Put ERRP_AUTO_PROPAGATE()
>> before its helpers, and touch up style. Commit message tweaked.]
>> Signed-off-by: Markus Armbruster<armbru@redhat.com>
>
> Ok, I see you have mostly rewritten the big comment
Guilty as charged... I was happy with the contents you provided (and
grateful for it), but our parallel work caused some redundancy. I went
beyond a minimal merge to get a something that reads as one coherent
text.
> and not only in this patch, so, I go and read the whole comment on top of
> these series.
>
> =================================
>
> * Pass an existing error to the caller with the message modified:
> * error_propagate_prepend(errp, err,
> * "Could not frobnicate '%s': ", name);
> * This is more concise than
> * error_propagate(errp, err); // don't do this
> * error_prepend(errp, "Could not frobnicate '%s': ", name);
> * and works even when @errp is &error_fatal.
>
> - the latter doesn't consider ERRP_AUTO_PROPAGATE: as we know, that
> ERRP_AUTO_PROPAGATE should be used when we use error_prepend, the latter
> should look like
>
>
> ERRP_AUTO_PROPAGATE();
> ...
> error_propagate(errp, err); // don't do this
> error_prepend(errp, "Could not frobnicate '%s': ", name);
>
> - and it works even when @errp is &error_fatal, so the
> error_propagate_prepend now is just a shortcut, not the only correct way.
I can duplicate the advice from the paragraph preceding it, like this:
* This is rarely needed. When @err is a local variable, use of
* ERRP_GUARD() commonly results in more readable code.
* Where it is needed, it is more concise than
* error_propagate(errp, err); // don't do this
* error_prepend(errp, "Could not frobnicate '%s': ", name);
* and works even when @errp is &error_fatal.
> Still, the text is formally correct as is, and may be improved later.
>
> =================================
>
> * 2. Replace &err by errp, and err by *errp. Delete local variable
> * @err.
>
> - hmm a bit not obvious,, It can be local_err.
Yes, but I trust the reader can make that mental jump.
> It can be (in some rare cases) still needed to handle the error locally, not
> passing to the caller..
I didn't think of this.
What about
* To convert a function to use ERRP_GUARD(), assuming the local
* variable it propagates to @errp is called @err:
[...]
* 2. Replace &err by errp, and err by *errp. Delete local variable
* @err if it s now unused.
Nope, still no good, if we replace like that, @err *will* be unused, and
the locally handled error will leak to the caller.
No time left for wordsmithing; let's improve on top.
> may be just something like "Assume local Error *err variable is used to get
> errors from called functions and than propagated to caller's errp" before
> paragraph [2.] will help.
>
>
> *
> * 3. Delete error_propagate(errp, *errp), replace
> * error_propagate_prepend(errp, *errp, ...) by error_prepend(errp, ...),
> *
> * 4. Ensure @errp is valid at return: when you destroy *errp, set
> * errp = NULL.
>
> =================================
>
>
> May be good to add note about ERRP_AUTO_PROPAGATE() into comment above
> error_append_hint (and error_(v)prepend)).
Good point.
> =================================
>
> /*
> * Make @errp parameter easier to use regardless of argument value
>
> may be s/argument/its/
>
> *
> * This macro is for use right at the beginning of a function that
> * takes an Error **errp parameter to pass errors to its caller. The
> * parameter must be named @errp.
> *
> * It must be used when the function dereferences @errp or passes
> * @errp to error_prepend(), error_vprepend(), or error_append_hint().
> * It is safe to use even when it's not needed, but please avoid
> * cluttering the source with useless code.
> *
> * If @errp is NULL or &error_fatal, rewrite it to point to a local
> * Error variable, which will be automatically propagated to the
> * original @errp on function exit.
> *
> * Note: &error_abort is not rewritten, because that would move the
> * abort from the place where the error is created to the place where
> * it's propagated.
> */
>
> =================================
>
>
> All these are minor, the documentation is good as is, thank you!
Thanks for your review, and your patience!
- [PATCH v12 0/8] error: auto propagated local_err part I, Markus Armbruster, 2020/07/07
- [PATCH v12 3/8] sd: Use ERRP_AUTO_PROPAGATE(), Markus Armbruster, 2020/07/07
- [PATCH v12 1/8] error: New macro ERRP_AUTO_PROPAGATE(), Markus Armbruster, 2020/07/07
- [PATCH v12 4/8] pflash: Use ERRP_AUTO_PROPAGATE(), Markus Armbruster, 2020/07/07
- [PATCH v12 8/8] xen: Use ERRP_AUTO_PROPAGATE(), Markus Armbruster, 2020/07/07
- [PATCH v12 6/8] virtio-9p: Use ERRP_AUTO_PROPAGATE(), Markus Armbruster, 2020/07/07
- [PATCH v12 2/8] scripts: Coccinelle script to use ERRP_AUTO_PROPAGATE(), Markus Armbruster, 2020/07/07
- [PATCH v12 5/8] fw_cfg: Use ERRP_AUTO_PROPAGATE(), Markus Armbruster, 2020/07/07
- [PATCH v12 7/8] nbd: Use ERRP_AUTO_PROPAGATE(), Markus Armbruster, 2020/07/07
- Re: [PATCH v12 0/8] error: auto propagated local_err part I, Vladimir Sementsov-Ogievskiy, 2020/07/07