Re: [PATCH v2 2/3] spapr: Use error_append_hint() in spapr_caps.c

[Laurent Vivier]

On 11/06/2020 12:44, Greg Kurz wrote:
> On Thu, 11 Jun 2020 13:42:48 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
>> 11.06.2020 13:21, Vladimir Sementsov-Ogievskiy wrote:
>>> 11.06.2020 13:13, Greg Kurz wrote:
>>>> On Thu, 11 Jun 2020 11:50:57 +0200
>>>> Laurent Vivier <lvivier@redhat.com> wrote:
>>>>
>>>>> On 11/06/2020 11:10, Greg Kurz wrote:
>>>>>> We have a dedicated error API for hints. Use it instead of embedding
>>>>>> the hint in the error message, as recommanded in the "qapi/error.h"
>>>>>> header file.
>>>>>>
>>>>>> Since spapr_caps_apply() passes &error_fatal, all functions must
>>>>>> also call the ERRP_AUTO_PROPAGATE() macro for error_append_hint()
>>>>>> to be functional.
>>>>>>
>>>>>> While here, add some missing braces around one line statements that
>>>>>> are part of the patch context. Also have cap_fwnmi_apply(), which
>>>>>> already uses error_append_hint() to call ERRP_AUTO_PROPAGATE() as
>>>>>> well.
>>>>>>
>>>>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>>>>> ---
>>>>>>   hw/ppc/spapr_caps.c |   95 
>>>>>> +++++++++++++++++++++++++++++----------------------
>>>>>>   1 file changed, 54 insertions(+), 41 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>>>>>> index efdc0dbbcfc0..2cb7ba8f005a 100644
>>>>>> --- a/hw/ppc/spapr_caps.c
>>>>>> +++ b/hw/ppc/spapr_caps.c
>>>>> ...
>>>>>> @@ -248,6 +249,7 @@ SpaprCapPossible cap_cfpc_possible = {
>>>>>>   static void cap_safe_cache_apply(SpaprMachineState *spapr, uint8_t val,
>>>>>>                                    Error **errp)
>>>>>>   {
>>>>>> +    ERRP_AUTO_PROPAGATE();
>>>>>>       Error *local_err = NULL;
>>>>>
>>>>> I think you should rename it, something like "local_warn" to not be
>>>>> confused with the _auto_errp_prop.local_err...
>>>>>
>>>>> or don't use ERRP_AUTO_PROPAGE(), use the local_err instead and move the
>>>>> warning inside the braces of the if.
>>>>>
>>>>> Same comment for cap_safe_bounds_check_apply() and
>>>>> cap_safe_indirect_branch_apply()
>>>>>
>>>>
>>>> Hmm... local_err isn't useful actually. It looks like we just want
>>>> to call warn_report() directly instead of error_setg(&local_err)
>>>> and warn_report_err(local_err). I'll post a v3.
>>>
>>> something like this I think:
>>>
>>> --- a/hw/ppc/spapr_caps.c
>>> +++ b/hw/ppc/spapr_caps.c
>>> @@ -250,24 +250,23 @@ static void cap_safe_cache_apply(SpaprMachineState 
>>> *spapr, uint8_t val,
>>>                                    Error **errp)
>>>   {
>>>       ERRP_AUTO_PROPAGATE();
>>> -    Error *local_err = NULL;
>>>       uint8_t kvm_val =  kvmppc_get_cap_safe_cache();
>>>
>>>       if (tcg_enabled() && val) {
>>>           /* TCG only supports broken, allow other values and print a 
>>> warning */
>>> -        error_setg(&local_err,
>>> +        error_setg(errp,
>>>                      "TCG doesn't support requested feature, cap-cfpc=%s",
>>>                      cap_cfpc_possible.vals[val]);
>>> +        if (*errp) {
>>> +            warn_report_err(*errp);
>>> +            *errp = NULL;
>>> +        }
>>
>> what a stupid code :) at least, if condition is always true.
>>
>> this all should be substitute by just
>>
>>      warn_report("TCG doesn't support requested feature, cap-cfpc=%s", 
>> cap_cfpc_possible.vals[val]);
>>
> 
> Exactly ! :)
> 

Yes, it seems appropriate.

Thanks,
Laurent