Re: [PATCH v6] error: rename errp to errp_in where it is IN-argument

[Vladimir Sementsov-Ogievskiy]

28.11.2019 23:29, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <address@hidden> writes:
> 
>> 28.11.2019 17:23, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <address@hidden> 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.
>>>>
>>>> But very few functions instead get Error **errp as IN-argument:
>>>> it's assumed to be set (or, maybe, NULL), and callee should clean it,
>>>> or add some information.
>>>>
>>>> In such cases, rename errp to errp_in.
>>>
>>> Missing: why is the rename useful?
>>
>> The main reason is to prepare for coccinelle part.
> 
> It's not a prerequisite for applying the patches Coccinelle produces,
> only a prerequisite for running Coccinelle.
> 
>>> It's useful if it helps readers recognize unusual Error ** parameters,
>>> and recognizing unusual Error ** parameters is actually a problem.  I'm
>>> not sure it is, but my familiarity with the Error interface may blind
>>> me.
>>>
>>> How many functions have unusual Error **parameters?  How are they used?
>>> Any calls that could easily be mistaken as the usual case?  See [*]
>>> below.
>>>
>>> You effectively propose a naming convention.  error.h should spell it
>>> out.  Let me try:
>>>
>>>       Any Error ** parameter meant for passing an error to the caller must
>>>       be named @errp.  No other Error ** parameter may be named @errp.
>>
>> Good
>>
>>>
>>> Observe:
>>>
>>> * I refrain from stipulating how other Error ** parameters are to be
>>>     named.  You use @errp_in, because the ones you rename are actually
>>>     "IN-arguments".  However, different uses are conceivable, where
>>>     @errp_in would be misleading.
>>>
>>> * If I understand your ERRP_AUTO_PROPAGATE() idea correctly, many
>>>     functions that take an Error ** to pass an error to the caller will
>>>     also use ERRP_AUTO_PROPAGATE, but not all.  Thus, presence of
>>>     ERRP_AUTO_PROPAGATE() won't be a reliable indicator of "the Error **
>>>     parameter is for passing an error to the caller".
>>>
>>> * I can't see machinery to help us catch violations of the convention.
>>>
>>>> This patch updates only error API functions. There still a few
>>>> functions with errp-in semantics, they will be updated in further
>>>> commits.
>>>
>>> Splitting the series into individual patches was a bad idea :)
>>>
>>> First, it really needs review as a whole.  I'll do that, but now I have
>>> to hunt down the parts.  Found so far:
>>>
>>>       [PATCH v6] error: rename errp to errp_in where it is IN-argument
>>>       [PATCH v6] hmp: drop Error pointer indirection in hmp_handle_error
>>>       [PATCH v6] vnc: drop Error pointer indirection in vnc_client_io_error
>>>       [PATCH v6] qdev-monitor: well form error hint helpers
>>>       [PATCH v6] nbd: well form nbd_iter_channel_error errp handler
>>>       [PATCH v6] ppc: well form kvmppc_hint_smt_possible error hint helper
>>>       [PATCH v6] 9pfs: well form error hint helpers
>>>       [PATCH v6] hw/core/qdev: cleanup Error ** variables
>>>       [PATCH v6] block/snapshot: rename Error ** parameter to more common 
>>> errp
>>>       [PATCH v6] hw/i386/amd_iommu: rename Error ** parameter to more 
>>> common errp
>>>       [PATCH v6] qga: rename Error ** parameter to more common errp
>>>       [PATCH v6] monitor/qmp-cmds: rename Error ** parameter to more common 
>>> errp
>>>       [PATCH v6] hw/s390x: rename Error ** parameter to more common errp
>>>       [PATCH v6] hw/sd: drop extra whitespace in sdhci_sysbus_realize() 
>>> header
>>>       [PATCH v6] hw/tpm: rename Error ** parameter to more common errp
>>>       [PATCH v6] hw/usb: rename Error ** parameter to more common errp
>>>       [PATCH v6] include/qom/object.h: rename Error ** parameter to more 
>>> common errp
>>>       [PATCH v6] backends/cryptodev: drop local_err from 
>>> cryptodev_backend_complete()
>>>       [PATCH v6] hw/vfio/ap: drop local_err from vfio_ap_realize
>>
>> .. 19 patches.. should be 21.
>>
>> It's really simple for me to resend them all in one v7 series. Should I?
> 
> Might add to the confusion.  Got a branch I can pull?
> 
>>>
>>> [*] The information I asked for above is buried in these patches.  I'll
>>> try to dig it up as I go reviewing them.
>>>
>>> Second, it risks some of these "further patches" overtake this one, and
>>> then its commit message will be misleading.  Moreover, the other commits
>>> will lack context.
>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>>> Reviewed-by: Eric Blake <address@hidden>
>>>> ---
>>>>
>>>> v6: fix s/errp/errp_in/ in comments corresponding to changed functions
>>>>       [Eric]
>>>>       add Eric's r-b
>>>>
>>>>    include/qapi/error.h | 16 ++++++++--------
>>>>    util/error.c         | 30 +++++++++++++++---------------
>>>>    2 files changed, 23 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>>> index 3f95141a01..df518644fc 100644
>>>> --- a/include/qapi/error.h
>>>> +++ b/include/qapi/error.h
>>>> @@ -230,16 +230,16 @@ void error_propagate_prepend(Error **dst_errp, Error 
>>>> *local_err,
>>>>                                 const char *fmt, ...);
>>>>    
>>>>    /*
>>>> - * Prepend some text to @errp's human-readable error message.
>>>> + * Prepend some text to @errp_in's human-readable error message.
>>>>     * The text is made by formatting @fmt, @ap like vprintf().
>>>>     */
>>>> -void error_vprepend(Error **errp, const char *fmt, va_list ap);
>>>> +void error_vprepend(Error **errp_in, const char *fmt, va_list ap);
>>>>    
>>>>    /*
>>>> - * Prepend some text to @errp's human-readable error message.
>>>> + * Prepend some text to @errp_in's human-readable error message.
>>>>     * The text is made by formatting @fmt, ... like printf().
>>>>     */
>>>> -void error_prepend(Error **errp, const char *fmt, ...)
>>>> +void error_prepend(Error **errp_in, const char *fmt, ...)
>>>>        GCC_FMT_ATTR(2, 3);
>>>>    
>>>>    /*
>>>> @@ -250,13 +250,13 @@ void error_prepend(Error **errp, const char *fmt, 
>>>> ...)
>>>>     * Intended use is adding helpful hints on the human user interface,
>>>>     * e.g. a list of valid values.  It's not for clarifying a confusing
>>>>     * error message.
>>>> - * @errp may be NULL, but not &error_fatal or &error_abort.
>>>> + * @errp_in may be NULL, but not &error_fatal or &error_abort.
>>>
>>> That's because the function modifies the error object.
>>>
>>> Hmm, so do error_prepend() and error_vprepend().  I figure we better
>>> update their contract accordingly, and copy the "not &error_fatal or
>>> &error_abort" assertion.  Not in this patch.  Maybe not even in this
>>> series.
>>>
>>>>     * Trivially the case if you call it only after error_setg() or
>>>>     * error_propagate().
>>>>     * May be called multiple times.  The resulting hint should end with a
>>>>     * newline.
>>>>     */
>>>> -void error_append_hint(Error **errp, const char *fmt, ...)
>>>> +void error_append_hint(Error **errp_in, const char *fmt, ...)
>>>>        GCC_FMT_ATTR(2, 3);
>>>>    
>>>>    /*
>>>> @@ -281,9 +281,9 @@ Error *error_copy(const Error *err);
>>>>    void error_free(Error *err);
>>>>    
>>>>    /*
>>>> - * Convenience function to assert that *@errp is set, then silently free 
>>>> it.
>>>> + * Convenience function to assert that *@errp_in is set, then silently 
>>>> free it.
>>> Long line.  Suggest:
>>>
>>>       * Assert that *@errp_in is set, then silently free it.
>>>       * This is a convenience function for use in tests.
>>>
>>>>     */
>>>> -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/util/error.c b/util/error.c
>>>> index d4532ce318..275586faa8 100644
>>>> --- a/util/error.c
>>>> +++ b/util/error.c
>>>> @@ -121,41 +121,41 @@ void error_setg_file_open_internal(Error **errp,
>>>>                                  "Could not open '%s'", filename);
>>>>    }
>>>>    
>>>> -void error_vprepend(Error **errp, const char *fmt, va_list ap)
>>>> +void error_vprepend(Error **errp_in, const char *fmt, va_list ap)
>>>>    {
>>>>        GString *newmsg;
>>>>    
>>>> -    if (!errp) {
>>>> +    if (!errp_in) {
>>>>            return;
>>>>    
>>>>        newmsg = g_string_new(NULL);
>>>>        g_string_vprintf(newmsg, fmt, ap);
>>>> -    g_string_append(newmsg, (*errp)->msg);
>>>> -    g_free((*errp)->msg);
>>>> -    (*errp)->msg = g_string_free(newmsg, 0);
>>>> +    g_string_append(newmsg, (*errp_in)->msg);
>>>> +    g_free((*errp_in)->msg);
>>>> +    (*errp_in)->msg = g_string_free(newmsg, 0);
>>>>    }
>>>>    
>>>> -void error_prepend(Error **errp, const char *fmt, ...)
>>>> +void error_prepend(Error **errp_in, const char *fmt, ...)
>>>>    {
>>>>        va_list ap;
>>>>    
>>>>        va_start(ap, fmt);
>>>> -    error_vprepend(errp, fmt, ap);
>>>> +    error_vprepend(errp_in, fmt, ap);
>>>>        va_end(ap);
>>>>    }
>>>>    
>>>> -void error_append_hint(Error **errp, const char *fmt, ...)
>>>> +void error_append_hint(Error **errp_in, const char *fmt, ...)
>>>>    {
>>>>        va_list ap;
>>>>        int saved_errno = errno;
>>>>        Error *err;
>>>>    
>>>> -    if (!errp) {
>>>> +    if (!errp_in) {
>>>>            return;
>>>>        }
>>>> -    err = *errp;
>>>> -    assert(err && errp != &error_abort && errp != &error_fatal);
>>>> +    err = *errp_in;
>>>> +    assert(err && errp_in != &error_abort && errp_in != &error_fatal);
>>>>    
>>>>        if (!err->hint) {
>>>>            err->hint = g_string_new(NULL);
>>>> @@ -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;
> 
> This one is actually in/out.
> 
> To make the compiler check errp_in is truly an in-argument, we can
> declare it as Error *const *errp_in.
> 
> But we can save ourselves the trouble of renaming it; the const should
> suffice to tell both human readers and Coccinelle that this is not your
> common out-argument.  I think I like this better than relying on a
> naming convention.  What about you?

I think it's good idea... But what to do with error_free_or_abort, and other 
functions
which get filled errp, and want to set it to NULL? Patch 21 adds three such 
functions..

Maybe, add assert(errp) at start of such functions, and catch it by coccinelle 
(not sure
that it is possible)?

> 
>>>>    }
>>>>    
>>>>    void error_propagate(Error **dst_errp, Error *local_err)
>>>
> 


-- 
Best regards,
Vladimir