08.10.2019 12:08, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsement...@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 <vsement...@virtuozzo.com>
>> Reviewed-by: Eric Blake <ebl...@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.
>
>
> I figure your commit message's "But very few functions instead get Error
> **errp as IN-argument: it's assumed to be set, and callee should clean
> it" is to be read as "a few functions take Error **errp as IN-argument,
> and free it".
>
> You found three instances of confusing Error **errp. How? I'm asking
> because I wonder whether there are more.
I don' remember exactly, but assume that I found them by compilation errors
after automatic conversion.
>
> We can avoid the confusing Error **errp in all three cases by peeling
> off an indirection. What do you think?
>
I like the idea, thanks! I think, I'll try to make a patch.
But you are right, unfortunately there more cases, at least, pointed by
Greg
error_append_socket_sockfd_hint and
error_append_security_model_hint
Which don't free error.. But if they append hint, they should always be called
on wrapped errp, accordingly to the problem about fatal_error, so they may
be converted to Error *err too.. Hmm, I should think about the script to find
such functions.
--
Best regards,
Vladimir
