[PATCH] qapi: define cleanup function for g_autoptr(Error)

[PATCH] qapi: define cleanup function for g_autoptr(Error)

[Paolo Bonzini]

Allow replacing calls to error_free() with g_autoptr(Error)
declarations.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qapi/error.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 4a9260b0cc..8564657baf 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -437,6 +437,8 @@ Error *error_copy(const Error *err);
  */
 void error_free(Error *err);
 
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free);
+
 /*
  * Convenience function to assert that *@errp is set, then silently free it.
  */
-- 
2.31.1

Re: [PATCH] qapi: define cleanup function for g_autoptr(Error)

[Richard Henderson]

On 9/12/21 5:48 AM, Paolo Bonzini wrote:
> Allow replacing calls to error_free() with g_autoptr(Error)
> declarations.
> 
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   include/qapi/error.h | 2 ++
>   1 file changed, 2 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH] qapi: define cleanup function for g_autoptr(Error)

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Allow replacing calls to error_free() with g_autoptr(Error)
> declarations.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qapi/error.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 4a9260b0cc..8564657baf 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -437,6 +437,8 @@ Error *error_copy(const Error *err);
>   */
>  void error_free(Error *err);
>  
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free);
> +
>  /*
>   * Convenience function to assert that *@errp is set, then silently free it.
>   */

I'd like to see at least one actual use.

Re: [PATCH] qapi: define cleanup function for g_autoptr(Error)

[Paolo Bonzini]

On 13/09/21 07:23, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Allow replacing calls to error_free() with g_autoptr(Error)
>> declarations.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   include/qapi/error.h | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index 4a9260b0cc..8564657baf 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -437,6 +437,8 @@ Error *error_copy(const Error *err);
>>    */
>>   void error_free(Error *err);
>>   
>> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free);
>> +
>>   /*
>>    * Convenience function to assert that *@errp is set, then silently free it.
>>    */
> 
> I'd like to see at least one actual use.

I'll have one soon, I'll Cc you on that one.  (I wrote this because Dan 
suggested using g_autoptr(Error) in a review, but it doesn't work yet).

Paolo

Re: [PATCH] qapi: define cleanup function for g_autoptr(Error)

[Daniel P. Berrangé]

On Mon, Sep 13, 2021 at 09:30:07AM +0200, Paolo Bonzini wrote:
> On 13/09/21 07:23, Markus Armbruster wrote:
> > Paolo Bonzini <pbonzini@redhat.com> writes:
> > 
> > > Allow replacing calls to error_free() with g_autoptr(Error)
> > > declarations.
> > > 
> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > > ---
> > >   include/qapi/error.h | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/include/qapi/error.h b/include/qapi/error.h
> > > index 4a9260b0cc..8564657baf 100644
> > > --- a/include/qapi/error.h
> > > +++ b/include/qapi/error.h
> > > @@ -437,6 +437,8 @@ Error *error_copy(const Error *err);
> > >    */
> > >   void error_free(Error *err);
> > > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free);
> > > +
> > >   /*
> > >    * Convenience function to assert that *@errp is set, then silently free it.
> > >    */
> > 
> > I'd like to see at least one actual use.
> 
> I'll have one soon, I'll Cc you on that one.  (I wrote this because Dan
> suggested using g_autoptr(Error) in a review, but it doesn't work yet).

Actually on reflection we probably don't need that because I forgot
that error_report_err free's the error object.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] qapi: define cleanup function for g_autoptr(Error)

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 13/09/21 07:23, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> Allow replacing calls to error_free() with g_autoptr(Error)
>>> declarations.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>   include/qapi/error.h | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>> index 4a9260b0cc..8564657baf 100644
>>> --- a/include/qapi/error.h
>>> +++ b/include/qapi/error.h
>>> @@ -437,6 +437,8 @@ Error *error_copy(const Error *err);
>>>    */
>>>   void error_free(Error *err);
>>>   +G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free);
>>> +
>>>   /*
>>>    * Convenience function to assert that *@errp is set, then silently free it.
>>>    */
>> I'd like to see at least one actual use.
>
> I'll have one soon, I'll Cc you on that one.  (I wrote this because
> Dan suggested using g_autoptr(Error) in a review, but it doesn't work
> yet).

I recommend to squash this patch into its first user, or maybe put it
right before it.

Re: [PATCH] qapi: define cleanup function for g_autoptr(Error)

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> On 13/09/21 07:23, Markus Armbruster wrote:
>>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>> 
>>>> Allow replacing calls to error_free() with g_autoptr(Error)
>>>> declarations.
>>>>
>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> ---
>>>>   include/qapi/error.h | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>>> index 4a9260b0cc..8564657baf 100644
>>>> --- a/include/qapi/error.h
>>>> +++ b/include/qapi/error.h
>>>> @@ -437,6 +437,8 @@ Error *error_copy(const Error *err);
>>>>    */
>>>>   void error_free(Error *err);
>>>>   +G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free);
>>>> +
>>>>   /*
>>>>    * Convenience function to assert that *@errp is set, then silently free it.
>>>>    */
>>> I'd like to see at least one actual use.
>>
>> I'll have one soon, I'll Cc you on that one.  (I wrote this because
>> Dan suggested using g_autoptr(Error) in a review, but it doesn't work
>> yet).
>
> I recommend to squash this patch into its first user, or maybe put it
> right before it.

I went for a walk, and now I have more substantial comments.

I'm not sure g_autoptr() is a good match for the Error interface in its
current shape.  Let me explain.

Use of error_free() is relatively rare: a bit over 200 calls outside
tests/, compared to more than 4000 error_setg*().  This is because the
most common ways to clean up are propagation and reporting, not
error_free().

As is, reporting errors doesn't play well with g_autoptr().  Example:

    Error *err = NULL;

    ... code that may set @err ...

    if (error is serious) {
        error_report_err(err);
    } else {
        error_free(err);
    }

Tempting, but wrong:

    g_autoptr(Error) err = NULL;

    ... code that may set @err ...

    if (error is serious) {
        error_report_err(err);
    }

Double-free, since error_report_err() frees its argument.  Correct:

    g_autoptr(Error) err = NULL;

    ... code that may set @err ...

    if (error is serious) {
        error_report_err(err);
        err = NULL;
    }

Hardly an improvement.

Same for propagation: replace error_report_err(err) by
error_propagate(errp, err).

If we decide we want g_autoptr(Error) anyway, then error.h's big comment
needs an update.

Re: [PATCH] qapi: define cleanup function for g_autoptr(Error)

[Philippe Mathieu-Daudé]

On 9/13/21 3:08 PM, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> On 13/09/21 07:23, Markus Armbruster wrote:
>>>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>>>
>>>>> Allow replacing calls to error_free() with g_autoptr(Error)
>>>>> declarations.
>>>>>
>>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>>> ---
>>>>>   include/qapi/error.h | 2 ++
>>>>>   1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>>>>> index 4a9260b0cc..8564657baf 100644
>>>>> --- a/include/qapi/error.h
>>>>> +++ b/include/qapi/error.h
>>>>> @@ -437,6 +437,8 @@ Error *error_copy(const Error *err);
>>>>>    */
>>>>>   void error_free(Error *err);
>>>>>   +G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free);
>>>>> +
>>>>>   /*
>>>>>    * Convenience function to assert that *@errp is set, then silently free it.
>>>>>    */
>>>> I'd like to see at least one actual use.
>>>
>>> I'll have one soon, I'll Cc you on that one.  (I wrote this because
>>> Dan suggested using g_autoptr(Error) in a review, but it doesn't work
>>> yet).
>>
>> I recommend to squash this patch into its first user, or maybe put it
>> right before it.
> 
> I went for a walk, and now I have more substantial comments.
> 
> I'm not sure g_autoptr() is a good match for the Error interface in its
> current shape.  Let me explain.
> 
> Use of error_free() is relatively rare: a bit over 200 calls outside
> tests/, compared to more than 4000 error_setg*().  This is because the
> most common ways to clean up are propagation and reporting, not
> error_free().
> 
> As is, reporting errors doesn't play well with g_autoptr().  Example:
> 
>     Error *err = NULL;
> 
>     ... code that may set @err ...
> 
>     if (error is serious) {
>         error_report_err(err);
>     } else {
>         error_free(err);
>     }

error_report_err() seems always called within an if()
statement, so an alternative is to refactor this pattern as:

  void error_report_err_cond(bool condition, Error *err);

Re: [PATCH] qapi: define cleanup function for g_autoptr(Error)

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 9/13/21 3:08 PM, Markus Armbruster wrote:
>> Markus Armbruster <armbru@redhat.com> writes:

[...]

>> As is, reporting errors doesn't play well with g_autoptr().  Example:
>> 
>>     Error *err = NULL;
>> 
>>     ... code that may set @err ...
>> 
>>     if (error is serious) {
>>         error_report_err(err);
>>     } else {
>>         error_free(err);
>>     }
>
> error_report_err() seems always called within an if()
> statement, so an alternative is to refactor this pattern as:
>
>   void error_report_err_cond(bool condition, Error *err);

It's rarely the only thing done when the condition is true, so it needs
to return it.