Re: [PATCH 03/12] hw/cxl: Convert cxl_fmws_link() to Error

[Markus Armbruster <armbru@redhat.com>]

Jonathan Cameron <Jonathan.Cameron@huawei.com> writes:

> On Fri,  8 Aug 2025 10:08:14 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Functions that use an Error **errp parameter to return errors should
>> not also report them to the user, because reporting is the caller's
>> job.  When the caller does, the error is reported twice.  When it
>> doesn't (because it recovered from the error), there is no error to
>> report, i.e. the report is bogus.
>> 
>> cxl_fmws_link_targets() violates this principle: it calls
>> error_setg(&error_fatal, ...) via cxl_fmws_link().  Goes back to
>> commit 584f722eb3ab (hw/cxl: Make the CXL fixed memory windows
>> devices.)  Currently harmless, because cxl_fmws_link_targets()'s
>> callers always pass &error_fatal.  Clean this up by converting
>> cxl_fmws_link() to Error.
>
> Patch is definitely an improvement though I'm no sure how
> it is really a violation of the above principle given
> it has no effect on being called twice for example.

Note I wrote "Clean this up", not "fix this" :)

This is actually a canned commit message I've been using with suitable
adjustments for similar patches: commit b765d21e4ab, 35b1561e3ec,
e6696d3ee9b, 07d5b946539, ...

>> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> The -1 return is perhaps unrelated to the main thing here,
> but does make more sense than return 1 so fair enough.

Accident, will back it out.

> None of the above comments I've raised are that important to me though.
>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>

Thanks!

>> ---
>>  hw/cxl/cxl-host.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
>> index 5c2ce25a19..0d891c651d 100644
>> --- a/hw/cxl/cxl-host.c
>> +++ b/hw/cxl/cxl-host.c
>> @@ -72,6 +72,7 @@ static void cxl_fixed_memory_window_config(CXLFixedMemoryWindowOptions *object,
>>  
>>  static int cxl_fmws_link(Object *obj, void *opaque)
>>  {
>> +    Error **errp = opaque;
>>      struct CXLFixedWindow *fw;
>>      int i;
>>  
>> @@ -87,9 +88,9 @@ static int cxl_fmws_link(Object *obj, void *opaque)
>>          o = object_resolve_path_type(fw->targets[i], TYPE_PXB_CXL_DEV,
>>                                       &ambig);
>>          if (!o) {
>> -            error_setg(&error_fatal, "Could not resolve CXLFM target %s",
>> +            error_setg(errp, "Could not resolve CXLFM target %s",
>>                         fw->targets[i]);
>> -            return 1;
>> +            return -1;

This line is the accident.

>>          }
>>          fw->target_hbs[i] = PXB_CXL_DEV(o);
>>      }
>> @@ -99,7 +100,7 @@ static int cxl_fmws_link(Object *obj, void *opaque)
>>  void cxl_fmws_link_targets(Error **errp)
>>  {
>>      /* Order doesn't matter for this, so no need to build list */
>> -    object_child_foreach_recursive(object_get_root(), cxl_fmws_link, NULL);
>> +    object_child_foreach_recursive(object_get_root(), cxl_fmws_link, errp);
>>  }
>>  
>>  static bool cxl_hdm_find_target(uint32_t *cache_mem, hwaddr addr,