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.
>
> 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.
None of the above comments I've raised are that important to me though.
Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com>
> ---
> 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;
> }
> 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,
