From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
Michael Roth <mdroth@linux.vnet.ibm.com>
Subject: Re: [PATCH v4 01/31] errp: rename errp to errp_in where it is IN-argument
Date: Wed, 9 Oct 2019 09:42:46 +0000 [thread overview]
Message-ID: <c393e6cc-ae34-78bb-2b7a-53ab6ed1dbae@virtuozzo.com> (raw)
In-Reply-To: <878spvmwns.fsf@dusky.pond.sub.org>
08.10.2019 12:08, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> Error **errp is almost always OUT-argument: it's assumed to be NULL, or
>> pointer to NULL-initialized pointer, or pointer to error_abort or
>> error_fatal, for callee to report error.
>
> Yes.
>
>> But very few functions instead get Error **errp as IN-argument:
>> it's assumed to be set, and callee should clean it.
>
> What do you mean by "callee should clean"? Let's see below.
>
>> In such cases, rename errp to errp_in.
>
> I acknowledge that errp arguments that don't have the usual meaning can
> be confusing.
>
> Naming can help, comments can help, but perhaps we can tweak the code to
> avoid the problem instead. Let's see:
>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>> include/monitor/hmp.h | 2 +-
>> include/qapi/error.h | 2 +-
>> ui/vnc.h | 2 +-
>> monitor/hmp-cmds.c | 8 ++++----
>> ui/vnc.c | 10 +++++-----
>> util/error.c | 8 ++++----
>> 6 files changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
>> index a0e9511440..f929814f1a 100644
>> --- a/include/monitor/hmp.h
>> +++ b/include/monitor/hmp.h
>> @@ -16,7 +16,7 @@
>>
>> #include "qemu/readline.h"
>>
>> -void hmp_handle_error(Monitor *mon, Error **errp);
>> +void hmp_handle_error(Monitor *mon, Error **errp_in);
>>
>> void hmp_info_name(Monitor *mon, const QDict *qdict);
>> void hmp_info_version(Monitor *mon, const QDict *qdict);
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index 3f95141a01..9376f59c35 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -283,7 +283,7 @@ void error_free(Error *err);
>> /*
>> * Convenience function to assert that *@errp is set, then silently free it.
>> */
>> -void error_free_or_abort(Error **errp);
>> +void error_free_or_abort(Error **errp_in);
>>
>> /*
>> * Convenience function to warn_report() and free @err.
>> diff --git a/ui/vnc.h b/ui/vnc.h
>> index fea79c2fc9..00e0b48f2f 100644
>> --- a/ui/vnc.h
>> +++ b/ui/vnc.h
>> @@ -547,7 +547,7 @@ uint32_t read_u32(uint8_t *data, size_t offset);
>>
>> /* Protocol stage functions */
>> void vnc_client_error(VncState *vs);
>> -size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp);
>> +size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in);
>>
>> void start_client_init(VncState *vs);
>> void start_auth_vnc(VncState *vs);
>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>> index b2551c16d1..941d5d0a45 100644
>> --- a/monitor/hmp-cmds.c
>> +++ b/monitor/hmp-cmds.c
>> @@ -60,11 +60,11 @@
>> #include <spice/enums.h>
>> #endif
>>
>> -void hmp_handle_error(Monitor *mon, Error **errp)
>> +void hmp_handle_error(Monitor *mon, Error **errp_in)
>> {
>> - assert(errp);
>> - if (*errp) {
>> - error_reportf_err(*errp, "Error: ");
>> + assert(errp_in);
>> + if (*errp_in) {
>> + error_reportf_err(*errp_in, "Error: ");
>> }
>> }
>
> This functions frees the error. It leaves nothing for the caller to
> clean up.
>
> All callers pass &ERR, where ERR is a local variable. Perhaps a more
> robust way to signal "@errp is not the usual out-argument" would be
> peeling off an indirection: pass ERR, drop the assertion.
>
>>
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index 87b8045afe..9d6384d9b1 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -1312,7 +1312,7 @@ void vnc_disconnect_finish(VncState *vs)
>> g_free(vs);
>> }
>>
>> -size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
>> +size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in)
>> {
>> if (ret <= 0) {
>> if (ret == 0) {
>> @@ -1320,14 +1320,14 @@ size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
>> vnc_disconnect_start(vs);
>> } else if (ret != QIO_CHANNEL_ERR_BLOCK) {
>> trace_vnc_client_io_error(vs, vs->ioc,
>> - errp ? error_get_pretty(*errp) :
>> + errp_in ? error_get_pretty(*errp_in) :
>> "Unknown");
>> vnc_disconnect_start(vs);
>> }
>>
>> - if (errp) {
>> - error_free(*errp);
>> - *errp = NULL;
>> + if (errp_in) {
>> + error_free(*errp_in);
>> + *errp_in = NULL;
>> }
>> return 0;
>> }
>
> This function isn't trivial, and lacks a contract, so let's figure out
> what it does and how it's used.
>
> @ret can be:
>
> * Zero
>
> Trace EOF, call vnc_disconnect_start(), free the error, return zero.
>
> Aside: freeing the error without looking at it feels odd. Can this
> happen?
>
> * Negative other than QIO_CHANNEL_ERR_BLOCK
>
> Trace the error if any, else "Unknown" error, call
> vnc_disconnect_start(), free the error if any, return zero.
>
> Note that we can't have errp && !*errp here, or else tracing crashes
> in error_get_pretty().
>
> * QIO_CHANNEL_ERR_BLOCK
>
> Free the error, return zero
>
> * Positive
>
> Do nothing, return @ret
>
> Callers pass one of the following:
>
> * ret = -1 and errp = NULL
>
> This uses case "Negative other than QIO_CHANNEL_ERR_BLOCK". Since
> error is null, it traces an "Unknown" error.
>
> * ret and &err, where ret = FUN(..., &err), and FUN is
> qio_channel_read() or qio_channel_write().
>
> qio_channel_read(), _write() are documented to return non-negative on
> success, QIO_CHANNEL_ERR_BLOCK on "would block", and -1 on other
> error. By convention, they set an error exactly when they fail,
> i.e. when they return a negative value.
>
> When qio_channel_read() / _write() succeed, we use case "Positive" or
> "Zero". We don't free the error, which is fine, as none was returned.
> Aside: I *guess* the channel is non-blocking, and "zero" can happen
> only when read hits EOF.
>
> When qio_channel_read() / _write() fail, we use one of the error
> cases.
>
> Looks like vnc_client_io_error() takes an error code @ret and an
> optional error object in @errp with additional details. If @ret is
> non-negative, @errp must be null or point to null. If @ret is negative,
> @errp must be null or point to non-null.
>
> vnc_client_io_error() frees the error. It leaves nothing for the caller
> to clean up.
>
> I think we can again peel off an indirection. The two kinds of calls
> become:
>
> * ret = -1 and err = NULL
>
> No textual change, but the NULL gets converted to Error * instead of
> Error **.
>
> * ret and err
>
> Pass the (possibly null) error object instead of a pointer to the
> local variable.
>
>> diff --git a/util/error.c b/util/error.c
>> index d4532ce318..b3ff3832d6 100644
>> --- a/util/error.c
>> +++ b/util/error.c
>> @@ -271,11 +271,11 @@ void error_free(Error *err)
>> }
>> }
>>
>> -void error_free_or_abort(Error **errp)
>> +void error_free_or_abort(Error **errp_in)
>> {
>> - assert(errp && *errp);
>> - error_free(*errp);
>> - *errp = NULL;
>> + assert(errp_in && *errp_in);
>> + error_free(*errp_in);
>> + *errp_in = NULL;
>> }
>>
>> void error_propagate(Error **dst_errp, Error *local_err)
>
> This functions frees the error. It leaves nothing for the caller to
> clean up.
>
> All callers pass &ERR, where ERR is a local variable. We can peel off
> an indirection.
But if we drop indirection, we'll have to set local variable to NULL by
hand. Is it good?
Look at test_keyval_parse_list() for example: it uses local err object
several times, so it depends on the fact that error_free_or_abort
sets pointer to NULL.
--
Best regards,
Vladimir
next prev parent reply other threads:[~2019-10-09 17:07 UTC|newest]
Thread overview: 81+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-01 15:52 [PATCH v4 00/31] error: auto propagated local_err Vladimir Sementsov-Ogievskiy
2019-10-01 15:52 ` [PATCH v4 01/31] errp: rename errp to errp_in where it is IN-argument Vladimir Sementsov-Ogievskiy
2019-10-08 9:08 ` Markus Armbruster
2019-10-08 9:30 ` Vladimir Sementsov-Ogievskiy
2019-10-08 12:05 ` Markus Armbruster
2019-10-09 10:08 ` Vladimir Sementsov-Ogievskiy
2019-10-09 18:48 ` Markus Armbruster
2019-10-09 9:42 ` Vladimir Sementsov-Ogievskiy [this message]
2019-10-09 18:51 ` Markus Armbruster
2019-10-01 15:52 ` [PATCH v4 02/31] hw/core/loader-fit: fix freeing errp in fit_load_fdt Vladimir Sementsov-Ogievskiy
2019-10-01 16:13 ` Eric Blake
2019-10-08 14:24 ` Markus Armbruster
2019-10-01 15:52 ` [PATCH v4 03/31] net/net: fix local variable shadowing in net_client_init Vladimir Sementsov-Ogievskiy
2019-10-08 14:34 ` Markus Armbruster
2019-10-01 15:52 ` [PATCH v4 04/31] error: auto propagated local_err Vladimir Sementsov-Ogievskiy
2019-10-01 16:17 ` Eric Blake
2019-10-02 10:15 ` Roman Kagan
2019-10-02 14:00 ` Eric Blake
2019-10-08 16:03 ` Markus Armbruster
2019-10-08 16:19 ` Greg Kurz
2019-10-08 18:24 ` Markus Armbruster
2019-10-09 8:04 ` Markus Armbruster
2019-10-09 8:17 ` Vladimir Sementsov-Ogievskiy
2019-10-09 19:09 ` Markus Armbruster
2019-10-01 15:52 ` [PATCH v4 05/31] scripts: add script to fix error_append_hint/error_prepend usage Vladimir Sementsov-Ogievskiy
2019-10-01 16:22 ` Eric Blake
2019-10-01 17:01 ` Vladimir Sementsov-Ogievskiy
2019-10-01 16:50 ` Eric Blake
2019-10-01 17:08 ` Eric Blake
2019-10-01 17:15 ` Vladimir Sementsov-Ogievskiy
2019-10-01 15:52 ` [PATCH v4 06/31] python: add commit-per-subsystem.py Vladimir Sementsov-Ogievskiy
2019-10-07 15:55 ` Cornelia Huck
2019-10-07 16:10 ` Vladimir Sementsov-Ogievskiy
2019-10-07 16:16 ` Cornelia Huck
2019-10-07 16:21 ` Daniel P. Berrangé
2019-10-07 17:15 ` Vladimir Sementsov-Ogievskiy
2019-10-01 15:52 ` [PATCH v4 07/31] s390: Fix error_append_hint/error_prepend usage Vladimir Sementsov-Ogievskiy
2019-10-07 15:58 ` Cornelia Huck
2019-10-09 7:42 ` Markus Armbruster
2019-10-11 15:33 ` Vladimir Sementsov-Ogievskiy
2019-10-01 15:52 ` [PATCH v4 08/31] ARM TCG CPUs: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:52 ` [PATCH v4 09/31] PowerPC " Vladimir Sementsov-Ogievskiy
2019-10-01 15:52 ` [PATCH v4 10/31] arm: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:52 ` [PATCH v4 11/31] SmartFusion2: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 12/31] ASPEED BMCs: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 13/31] Boston: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 14/31] PowerNV (Non-Virtualized): " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 15/31] PCI: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 16/31] SCSI: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 17/31] USB: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 18/31] VFIO: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 19/31] vhost: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 20/31] virtio: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 21/31] virtio-9p: " Vladimir Sementsov-Ogievskiy
2019-10-02 9:19 ` Greg Kurz
2019-10-02 12:58 ` Vladimir Sementsov-Ogievskiy
2019-10-02 13:11 ` Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 22/31] XIVE: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 23/31] block: " Vladimir Sementsov-Ogievskiy
2019-10-01 17:09 ` Eric Blake
2019-10-01 18:55 ` Vladimir Sementsov-Ogievskiy
2019-10-01 19:12 ` Vladimir Sementsov-Ogievskiy
2019-10-01 19:44 ` Eric Blake
2019-10-09 7:22 ` Markus Armbruster
2019-10-01 15:53 ` [PATCH v4 24/31] chardev: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 25/31] cmdline: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 26/31] QOM: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 27/31] Migration: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 28/31] Sockets: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 29/31] nbd: " Vladimir Sementsov-Ogievskiy
2019-10-01 17:47 ` Eric Blake
2019-10-01 15:53 ` [PATCH v4 30/31] PVRDMA: " Vladimir Sementsov-Ogievskiy
2019-10-01 15:53 ` [PATCH v4 31/31] ivshmem: " Vladimir Sementsov-Ogievskiy
2019-10-02 3:26 ` [PATCH v4 00/31] error: auto propagated local_err no-reply
2019-10-02 11:58 ` Markus Armbruster
2019-10-08 7:30 ` Markus Armbruster
2019-10-08 8:41 ` Vladimir Sementsov-Ogievskiy
2019-10-08 9:39 ` Greg Kurz
2019-10-08 10:09 ` Vladimir Sementsov-Ogievskiy
2019-10-08 11:59 ` Markus Armbruster
2019-10-09 8:45 ` Vladimir Sementsov-Ogievskiy
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=c393e6cc-ae34-78bb-2b7a-53ab6ed1dbae@virtuozzo.com \
--to=vsementsov@virtuozzo.com \
--cc=armbru@redhat.com \
--cc=dgilbert@redhat.com \
--cc=kraxel@redhat.com \
--cc=mdroth@linux.vnet.ibm.com \
--cc=qemu-devel@nongnu.org \
/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).