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

[Jonathan Cameron via <qemu-devel@nongnu.org>]

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.

> 
> 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.

None of the above comments I've raised are that important to me though.

Reviewed-by: Jonathan Cameron <jonathan.cameron@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,