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.

This patch updates only error API functions. There still a few
functions with errp-in semantics, they will be updated in further
commits.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---

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.
  * 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.
  */
-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;
 }
 
 void error_propagate(Error **dst_errp, Error *local_err)
-- 
2.21.0

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

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.

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

[*] 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 <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>
> 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;
>  }
>  
>  void error_propagate(Error **dst_errp, Error *local_err)

28.11.2019 17:23, 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.
>>
>> 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 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?

> 
> [*] 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 <vsementsov@virtuozzo.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>
>> 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;
>>   }
>>   
>>   void error_propagate(Error **dst_errp, Error *local_err)
> 


-- 
Best regards,
Vladimir

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 28.11.2019 17:23, 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.
>>>
>>> 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 <vsementsov@virtuozzo.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>
>>> 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?

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

28.11.2019 23:29, Markus Armbruster wrote:
>>>       [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?
> 

preparations # v6:

https://src.openvz.org/scm/~vsementsov/qemu.git up-auto-local-err-preparations-v6
https://src.openvz.org/users/vsementsov/repos/qemu/browse?at=refs%2Ftags%2Fup-auto-local-err-preparations-v6

the whole series # v5:

https://src.openvz.org/scm/~vsementsov/qemu.git up-auto-local-err-v5
https://src.openvz.org/users/vsementsov/repos/qemu/browse?at=refs%2Ftags%2Fup-auto-local-err-v5

-- 
Best regards,
Vladimir

28.11.2019 23:29, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> 28.11.2019 17:23, 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.
>>>>
>>>> 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 <vsementsov@virtuozzo.com>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>> ---
>>>>
>>>> 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

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 28.11.2019 23:29, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>> 
>>> 28.11.2019 17:23, 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.
>>>>>
>>>>> 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.
[...]
>>>> [*] 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.
[...]
>>>>> diff --git a/util/error.c b/util/error.c
>>>>> index d4532ce318..275586faa8 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;
>> 
>> 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)?

I went through the git branch you provided.

These get rid of unusual Error ** parameters:

    01e10667d1 hmp: drop Error pointer indirection in hmp_handle_error
    606bfb7520 vnc: drop Error pointer indirection in vnc_client_io_error

    Get rid of them by peeling off an indirection.

These try to make unusual Error ** parameters stand out:

    51e73b3305 error: rename errp to errp_in where it is IN-argument

    Four renames.

    Three functions modify the error, name @errp_in is okay, my const
    proposal works.

    error_free_or_abort() clears *errp_in, name @errp_in is misleading,
    const doesn't work.

    f7bdfd42da qdev-monitor: well form error hint helpers

    Two renames.  Both functions modify the error, rename is okay, const
    works.

    9d4aac7299 nbd: well form nbd_iter_channel_error errp handler

    One rename, from @local_err.  nbd_iter_channel_error() clears *errp_in,
    name @errp_in is misleading, const doesn't work.

    fb1bd83c40 ppc: well form kvmppc_hint_smt_possible error hint helper

    One rename.  The function modify the error, rename is okay, const works.

    e024e89b10 9pfs: well form error hint helpers

    Two renames.  Both functions modify the error, rename is okay, const
    works.

These make usual Error ** parameters look, well, more usual:

    c01649d999 hw/core/qdev: cleanup Error ** variables
    a5f6424163 block/snapshot: rename Error ** parameter to more common errp
    ae200ca21e hw/i386/amd_iommu: rename Error ** parameter to more common errp
    561f73e681 qga: rename Error ** parameter to more common errp
    12589a96cd monitor/qmp-cmds: rename Error ** parameter to more common errp
    f608fc5999 hw/s390x: rename Error ** parameter to more common errp
    747a90d044 hw/tpm: rename Error ** parameter to more common errp
    3d19e66610 hw/usb: rename Error ** parameter to more common errp
    07366648ef include/qom/object.h: rename Error ** parameter to more common errp

These don't touch Error ** parameter declarations:

    f6e4fffc16 hw/core/loader-fit: fix freeing errp in fit_load_fdt
    b5bba880ae net/net: Clean up variable shadowing in net_client_init()
    4a4462ce4c hw/sd: drop extra whitespace in sdhci_sysbus_realize() header
    d52d44e822 backends/cryptodev: drop local_err from cryptodev_backend_complete()
    7e95d30766 hw/vfio/ap: drop local_err from vfio_ap_realize

Unusual Error ** parameters that can be made Error *const *errp should
not also need a rename, neither to avoid confusion about @errp's role,
nor to help Coccinelle.

Unusual Error ** parameters that can't be made Error *const *errp:

    nbd_iter_channel_error(): parameter is called @local_err.  Confusion
    seems as unlikely without the rename as with it.  Coccinelle should
    not need the rename.  No need to touch.  I'm willing to accept your
    assertion change.

    error_free_or_abort(): parameter is called @errp.  Confusion seems
    as unlikely without the rename as with it.  Coccinelle should not
    need the rename.  I'm willing to accept a rename to @local_err
    regardless.

I pushed my fixups to git://repo.or.cz/qemu/armbru.git branch error-prep
for your convenience.  The commit messages of the fixed up commits need
rephrasing, of course.

29.11.2019 17:35, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> 28.11.2019 23:29, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>
>>>> 28.11.2019 17:23, 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.
>>>>>>
>>>>>> 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.
> [...]
>>>>> [*] 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.
> [...]
>>>>>> diff --git a/util/error.c b/util/error.c
>>>>>> index d4532ce318..275586faa8 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;
>>>
>>> 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)?
> 
> I went through the git branch you provided.
> 
> These get rid of unusual Error ** parameters:
> 
>      01e10667d1 hmp: drop Error pointer indirection in hmp_handle_error
>      606bfb7520 vnc: drop Error pointer indirection in vnc_client_io_error
> 
>      Get rid of them by peeling off an indirection.
> 
> These try to make unusual Error ** parameters stand out:
> 
>      51e73b3305 error: rename errp to errp_in where it is IN-argument
> 
>      Four renames.
> 
>      Three functions modify the error, name @errp_in is okay, my const
>      proposal works.
> 
>      error_free_or_abort() clears *errp_in, name @errp_in is misleading,
>      const doesn't work.
> 
>      f7bdfd42da qdev-monitor: well form error hint helpers
> 
>      Two renames.  Both functions modify the error, rename is okay, const
>      works.
> 
>      9d4aac7299 nbd: well form nbd_iter_channel_error errp handler
> 
>      One rename, from @local_err.  nbd_iter_channel_error() clears *errp_in,
>      name @errp_in is misleading, const doesn't work.
> 
>      fb1bd83c40 ppc: well form kvmppc_hint_smt_possible error hint helper
> 
>      One rename.  The function modify the error, rename is okay, const works.
> 
>      e024e89b10 9pfs: well form error hint helpers
> 
>      Two renames.  Both functions modify the error, rename is okay, const
>      works.
> 
> These make usual Error ** parameters look, well, more usual:
> 
>      c01649d999 hw/core/qdev: cleanup Error ** variables
>      a5f6424163 block/snapshot: rename Error ** parameter to more common errp
>      ae200ca21e hw/i386/amd_iommu: rename Error ** parameter to more common errp
>      561f73e681 qga: rename Error ** parameter to more common errp
>      12589a96cd monitor/qmp-cmds: rename Error ** parameter to more common errp
>      f608fc5999 hw/s390x: rename Error ** parameter to more common errp
>      747a90d044 hw/tpm: rename Error ** parameter to more common errp
>      3d19e66610 hw/usb: rename Error ** parameter to more common errp
>      07366648ef include/qom/object.h: rename Error ** parameter to more common errp
> 
> These don't touch Error ** parameter declarations:
> 
>      f6e4fffc16 hw/core/loader-fit: fix freeing errp in fit_load_fdt
>      b5bba880ae net/net: Clean up variable shadowing in net_client_init()
>      4a4462ce4c hw/sd: drop extra whitespace in sdhci_sysbus_realize() header
>      d52d44e822 backends/cryptodev: drop local_err from cryptodev_backend_complete()
>      7e95d30766 hw/vfio/ap: drop local_err from vfio_ap_realize
> 
> Unusual Error ** parameters that can be made Error *const *errp should
> not also need a rename, neither to avoid confusion about @errp's role,
> nor to help Coccinelle.
> 
> Unusual Error ** parameters that can't be made Error *const *errp:
> 
>      nbd_iter_channel_error(): parameter is called @local_err.  Confusion
>      seems as unlikely without the rename as with it.  Coccinelle should
>      not need the rename.  No need to touch.  I'm willing to accept your
>      assertion change.

Hmm but you reverted it.. Will you keep an assertion?

> 
>      error_free_or_abort(): parameter is called @errp.  Confusion seems
>      as unlikely without the rename as with it.  Coccinelle should not
>      need the rename.  I'm willing to accept a rename to @local_err
>      regardless.

So, either we rename it to local_err, or coccinelle should distinguish it
by assertion at the top.

> 
> I pushed my fixups to git://repo.or.cz/qemu/armbru.git branch error-prep
> for your convenience.  The commit messages of the fixed up commits need
> rephrasing, of course.
> 

Looked through fixups, looks OK for me, thanks! What next?


-- 
Best regards,
Vladimir

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 29.11.2019 17:35, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>> 
>>> 28.11.2019 23:29, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>>
>>>>> 28.11.2019 17:23, 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.
>>>>>>>
>>>>>>> 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.
>> [...]
>>>>>> [*] 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.
>> [...]
>>>>>>> diff --git a/util/error.c b/util/error.c
>>>>>>> index d4532ce318..275586faa8 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;
>>>>
>>>> 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)?
>> 
>> I went through the git branch you provided.
>> 
>> These get rid of unusual Error ** parameters:
>> 
>>      01e10667d1 hmp: drop Error pointer indirection in hmp_handle_error
>>      606bfb7520 vnc: drop Error pointer indirection in vnc_client_io_error
>> 
>>      Get rid of them by peeling off an indirection.
>> 
>> These try to make unusual Error ** parameters stand out:
>> 
>>      51e73b3305 error: rename errp to errp_in where it is IN-argument
>> 
>>      Four renames.
>> 
>>      Three functions modify the error, name @errp_in is okay, my const
>>      proposal works.
>> 
>>      error_free_or_abort() clears *errp_in, name @errp_in is misleading,
>>      const doesn't work.
>> 
>>      f7bdfd42da qdev-monitor: well form error hint helpers
>> 
>>      Two renames.  Both functions modify the error, rename is okay, const
>>      works.
>> 
>>      9d4aac7299 nbd: well form nbd_iter_channel_error errp handler
>> 
>>      One rename, from @local_err.  nbd_iter_channel_error() clears *errp_in,
>>      name @errp_in is misleading, const doesn't work.
>> 
>>      fb1bd83c40 ppc: well form kvmppc_hint_smt_possible error hint helper
>> 
>>      One rename.  The function modify the error, rename is okay, const works.
>> 
>>      e024e89b10 9pfs: well form error hint helpers
>> 
>>      Two renames.  Both functions modify the error, rename is okay, const
>>      works.
>> 
>> These make usual Error ** parameters look, well, more usual:
>> 
>>      c01649d999 hw/core/qdev: cleanup Error ** variables
>>      a5f6424163 block/snapshot: rename Error ** parameter to more common errp
>>      ae200ca21e hw/i386/amd_iommu: rename Error ** parameter to more common errp
>>      561f73e681 qga: rename Error ** parameter to more common errp
>>      12589a96cd monitor/qmp-cmds: rename Error ** parameter to more common errp
>>      f608fc5999 hw/s390x: rename Error ** parameter to more common errp
>>      747a90d044 hw/tpm: rename Error ** parameter to more common errp
>>      3d19e66610 hw/usb: rename Error ** parameter to more common errp
>>      07366648ef include/qom/object.h: rename Error ** parameter to more common errp
>> 
>> These don't touch Error ** parameter declarations:
>> 
>>      f6e4fffc16 hw/core/loader-fit: fix freeing errp in fit_load_fdt
>>      b5bba880ae net/net: Clean up variable shadowing in net_client_init()
>>      4a4462ce4c hw/sd: drop extra whitespace in sdhci_sysbus_realize() header
>>      d52d44e822 backends/cryptodev: drop local_err from cryptodev_backend_complete()
>>      7e95d30766 hw/vfio/ap: drop local_err from vfio_ap_realize
>> 
>> Unusual Error ** parameters that can be made Error *const *errp should
>> not also need a rename, neither to avoid confusion about @errp's role,
>> nor to help Coccinelle.
>> 
>> Unusual Error ** parameters that can't be made Error *const *errp:
>> 
>>      nbd_iter_channel_error(): parameter is called @local_err.  Confusion
>>      seems as unlikely without the rename as with it.  Coccinelle should
>>      not need the rename.  No need to touch.  I'm willing to accept your
>>      assertion change.
>
> Hmm but you reverted it.. Will you keep an assertion?

You decide whether to change the assertion in v7.

>>      error_free_or_abort(): parameter is called @errp.  Confusion seems
>>      as unlikely without the rename as with it.  Coccinelle should not
>>      need the rename.  I'm willing to accept a rename to @local_err
>>      regardless.
>
> So, either we rename it to local_err, or coccinelle should distinguish it
> by assertion at the top.

Yes.

I'm also willing to consider a rename to a name other than @local_err,
if you can come up with a good, non-misleading one.

>> I pushed my fixups to git://repo.or.cz/qemu/armbru.git branch error-prep
>> for your convenience.  The commit messages of the fixed up commits need
>> rephrasing, of course.
>> 
>
> Looked through fixups, looks OK for me, thanks! What next?

Let me finish my fixing incorrect dereferences of errp, and then we
figure out what to include in v7.

Markus Armbruster <armbru@redhat.com> writes:

> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> 29.11.2019 17:35, Markus Armbruster wrote:
[...]
>>> I pushed my fixups to git://repo.or.cz/qemu/armbru.git branch error-prep
>>> for your convenience.  The commit messages of the fixed up commits need
>>> rephrasing, of course.
>>> 
>>
>> Looked through fixups, looks OK for me, thanks! What next?
>
> Let me finish my fixing incorrect dereferences of errp, and then we
> figure out what to include in v7.

Your v6 with my fixups does not conflict with my "[PATCH v2 00/18] Error
handling fixes", except for "hw/core/loader-fit: fix freeing errp in
fit_load_fdt", which my "[PATCH v2 07/18] hw/core: Fix fit_load_fdt()
error handling violations" supersedes.

Suggest you work in the fixups and post as v7.  I'll merge that in my
tree, to give you a base for the remainder of your "auto propagated
local_err" work.  While you work on that, I'll work on getting the base
merged into master.  Sounds like a plan?

04.12.2019 16:03, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> 29.11.2019 17:35, Markus Armbruster wrote:
> [...]
>>>> I pushed my fixups to git://repo.or.cz/qemu/armbru.git branch error-prep
>>>> for your convenience.  The commit messages of the fixed up commits need
>>>> rephrasing, of course.
>>>>
>>>
>>> Looked through fixups, looks OK for me, thanks! What next?
>>
>> Let me finish my fixing incorrect dereferences of errp, and then we
>> figure out what to include in v7.
> 
> Your v6 with my fixups does not conflict with my "[PATCH v2 00/18] Error
> handling fixes", except for "hw/core/loader-fit: fix freeing errp in
> fit_load_fdt", which my "[PATCH v2 07/18] hw/core: Fix fit_load_fdt()
> error handling violations" supersedes.
> 
> Suggest you work in the fixups and post as v7.  I'll merge that in my
> tree, to give you a base for the remainder of your "auto propagated
> local_err" work.  While you work on that, I'll work on getting the base
> merged into master.  Sounds like a plan?
> 

Yes, that's good. I'll send v7 tomorrow.

What you suggest to do after it?
Send in one series a patch with macro + coccinelle +
subset of autogenerated patches, which were reviewed (but not sending half
a subsystem of course)?

-- 
Best regards,
Vladimir

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 04.12.2019 16:03, Markus Armbruster wrote:
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>
>>>> 29.11.2019 17:35, Markus Armbruster wrote:
>> [...]
>>>>> I pushed my fixups to git://repo.or.cz/qemu/armbru.git branch error-prep
>>>>> for your convenience.  The commit messages of the fixed up commits need
>>>>> rephrasing, of course.
>>>>>
>>>>
>>>> Looked through fixups, looks OK for me, thanks! What next?
>>>
>>> Let me finish my fixing incorrect dereferences of errp, and then we
>>> figure out what to include in v7.
>> 
>> Your v6 with my fixups does not conflict with my "[PATCH v2 00/18] Error
>> handling fixes", except for "hw/core/loader-fit: fix freeing errp in
>> fit_load_fdt", which my "[PATCH v2 07/18] hw/core: Fix fit_load_fdt()
>> error handling violations" supersedes.
>> 
>> Suggest you work in the fixups and post as v7.  I'll merge that in my
>> tree, to give you a base for the remainder of your "auto propagated
>> local_err" work.  While you work on that, I'll work on getting the base
>> merged into master.  Sounds like a plan?
>> 
>
> Yes, that's good. I'll send v7 tomorrow.
>
> What you suggest to do after it?
> Send in one series a patch with macro + coccinelle +
> subset of autogenerated patches, which were reviewed (but not sending half
> a subsystem of course)?

Sounds good to me.

Visibility into the complete work is useful, though.  Having the cover
letter point to a branch in your public git repo should do.