qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
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

  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).