1. Patchew
  2. QEMU
  3. View series

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

Paolo Bonzini posted 1 patch 2 years, 7 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210912124834.503032-1-pbonzini@redhat.com
Maintainers: Michael Roth <michael.roth@amd.com>, Markus Armbruster <armbru@redhat.com>
include/qapi/error.h | 2 ++
1 file changed, 2 insertions(+)
[PATCH] qapi: define cleanup function for g_autoptr(Error)
Posted by Paolo Bonzini 2 years, 7 months ago 
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)
Posted by Richard Henderson 2 years, 7 months ago 
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)
Posted by Markus Armbruster 2 years, 7 months ago 
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)
Posted by Paolo Bonzini 2 years, 7 months ago 
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)
Posted by Markus Armbruster 2 years, 7 months ago 
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)
Posted by Markus Armbruster 2 years, 7 months ago 
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)
Posted by Philippe Mathieu-Daudé 2 years, 7 months ago 
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)
Posted by Markus Armbruster 2 years, 7 months ago 
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.

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.