Jonathan Cameron <jonathan.came...@huawei.com> writes:
> On Fri, 8 Aug 2025 10:08:14 +0200
> Markus Armbruster <arm...@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.came...@huawei.com>
>> Signed-off-by: Markus Armbruster <arm...@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.came...@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,
