[PATCH 0/3] hw/cxl/cxl-cdat: Make cxl_doe_cdat_init() return boolean

[PATCH 0/3] hw/cxl/cxl-cdat: Make cxl_doe_cdat_init() return boolean

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

Hi list,

This series is the followup of the previous fix [1] to improve the error
handling.

As error.h suggested, the best practice for callee is to return
something to indicate success / failure instead of dereferencing @errp.

Thus make cxl_doe_cdat_init() as well as ct3_load_cdat() and
ct3_build_cdat() return boolean.

[1]: https://lore.kernel.org/qemu-devel/20240221094317.994454-1-zhao1.liu@linux.intel.com/

Thanks,
Zhao

---
Zhao Liu (3):
  hw/cxl/cxl-cdat: Make ct3_load_cdat() return boolean
  hw/cxl/cxl-cdat: Make ct3_build_cdat() return boolean
  hw/cxl/cxl-cdat: Make cxl_doe_cdat_init() return boolean

 hw/cxl/cxl-cdat.c              | 28 +++++++++++++++-------------
 hw/mem/cxl_type3.c             |  3 +--
 hw/pci-bridge/cxl_upstream.c   |  3 +--
 include/hw/cxl/cxl_component.h |  2 +-
 4 files changed, 18 insertions(+), 18 deletions(-)

-- 
2.34.1

[PATCH 1/3] hw/cxl/cxl-cdat: Make ct3_load_cdat() return boolean

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

As error.h suggested, the best practice for callee is to return
something to indicate success / failure.

So make ct3_load_cdat() return boolean, and this is the preparation for
cxl_doe_cdat_init() returning boolean.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/cxl/cxl-cdat.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
index 551545f7823b..b3e496857a77 100644
--- a/hw/cxl/cxl-cdat.c
+++ b/hw/cxl/cxl-cdat.c
@@ -111,7 +111,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
     cdat->entry = g_steal_pointer(&cdat_st);
 }
 
-static void ct3_load_cdat(CDATObject *cdat, Error **errp)
+static bool ct3_load_cdat(CDATObject *cdat, Error **errp)
 {
     g_autofree CDATEntry *cdat_st = NULL;
     g_autofree uint8_t *buf = NULL;
@@ -127,11 +127,11 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
                              &file_size, &error)) {
         error_setg(errp, "CDAT: File read failed: %s", error->message);
         g_error_free(error);
-        return;
+        return false;
     }
     if (file_size < sizeof(CDATTableHeader)) {
         error_setg(errp, "CDAT: File too short");
-        return;
+        return false;
     }
     i = sizeof(CDATTableHeader);
     num_ent = 1;
@@ -139,19 +139,19 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
         hdr = (CDATSubHeader *)(buf + i);
         if (i + sizeof(CDATSubHeader) > file_size) {
             error_setg(errp, "CDAT: Truncated table");
-            return;
+            return false;
         }
         cdat_len_check(hdr, errp);
         i += hdr->length;
         if (i > file_size) {
             error_setg(errp, "CDAT: Truncated table");
-            return;
+            return false;
         }
         num_ent++;
     }
     if (i != file_size) {
         error_setg(errp, "CDAT: File length mismatch");
-        return;
+        return false;
     }
 
     cdat_st = g_new0(CDATEntry, num_ent);
@@ -185,6 +185,7 @@ static void ct3_load_cdat(CDATObject *cdat, Error **errp)
     cdat->entry_len = num_ent;
     cdat->entry = g_steal_pointer(&cdat_st);
     cdat->buf = g_steal_pointer(&buf);
+    return true;
 }
 
 void cxl_doe_cdat_init(CXLComponentState *cxl_cstate, Error **errp)
-- 
2.34.1

[PATCH 2/3] hw/cxl/cxl-cdat: Make ct3_build_cdat() return boolean

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

As error.h suggested, the best practice for callee is to return
something to indicate success / failure.

So make ct3_build_cdat() return boolean, and this is the preparation for
cxl_doe_cdat_init() returning boolean.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/cxl/cxl-cdat.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
index b3e496857a77..e7bc1380bfbf 100644
--- a/hw/cxl/cxl-cdat.c
+++ b/hw/cxl/cxl-cdat.c
@@ -44,7 +44,7 @@ static void cdat_len_check(CDATSubHeader *hdr, Error **errp)
     }
 }
 
-static void ct3_build_cdat(CDATObject *cdat, Error **errp)
+static bool ct3_build_cdat(CDATObject *cdat, Error **errp)
 {
     g_autofree CDATTableHeader *cdat_header = NULL;
     g_autofree CDATEntry *cdat_st = NULL;
@@ -58,7 +58,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
     cdat_header = g_malloc0(sizeof(*cdat_header));
     if (!cdat_header) {
         error_setg(errp, "Failed to allocate CDAT header");
-        return;
+        return false;
     }
 
     cdat->built_buf_len = cdat->build_cdat_table(&cdat->built_buf,
@@ -67,14 +67,14 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
     if (cdat->built_buf_len <= 0) {
         /* Build later as not all data available yet */
         cdat->to_update = true;
-        return;
+        return true;
     }
     cdat->to_update = false;
 
     cdat_st = g_malloc0(sizeof(*cdat_st) * (cdat->built_buf_len + 1));
     if (!cdat_st) {
         error_setg(errp, "Failed to allocate CDAT entry array");
-        return;
+        return false;
     }
 
     /* Entry 0 for CDAT header, starts with Entry 1 */
@@ -109,6 +109,7 @@ static void ct3_build_cdat(CDATObject *cdat, Error **errp)
     cdat_st[0].length = sizeof(*cdat_header);
     cdat->entry_len = 1 + cdat->built_buf_len;
     cdat->entry = g_steal_pointer(&cdat_st);
+    return true;
 }
 
 static bool ct3_load_cdat(CDATObject *cdat, Error **errp)
-- 
2.34.1

[PATCH 3/3] hw/cxl/cxl-cdat: Make cxl_doe_cdat_init() return boolean

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

As error.h suggested, the best practice for callee is to return
something to indicate success / failure.

With returned boolean, there's no need to dereference @errp to check
failure case.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/cxl/cxl-cdat.c              | 6 +++---
 hw/mem/cxl_type3.c             | 3 +--
 hw/pci-bridge/cxl_upstream.c   | 3 +--
 include/hw/cxl/cxl_component.h | 2 +-
 4 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
index e7bc1380bfbf..959a55518e65 100644
--- a/hw/cxl/cxl-cdat.c
+++ b/hw/cxl/cxl-cdat.c
@@ -189,14 +189,14 @@ static bool ct3_load_cdat(CDATObject *cdat, Error **errp)
     return true;
 }
 
-void cxl_doe_cdat_init(CXLComponentState *cxl_cstate, Error **errp)
+bool cxl_doe_cdat_init(CXLComponentState *cxl_cstate, Error **errp)
 {
     CDATObject *cdat = &cxl_cstate->cdat;
 
     if (cdat->filename) {
-        ct3_load_cdat(cdat, errp);
+        return ct3_load_cdat(cdat, errp);
     } else {
-        ct3_build_cdat(cdat, errp);
+        return ct3_build_cdat(cdat, errp);
     }
 }
 
diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index b0a7e9f11b64..3e42490b6ce8 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -705,8 +705,7 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
     cxl_cstate->cdat.build_cdat_table = ct3_build_cdat_table;
     cxl_cstate->cdat.free_cdat_table = ct3_free_cdat_table;
     cxl_cstate->cdat.private = ct3d;
-    cxl_doe_cdat_init(cxl_cstate, errp);
-    if (*errp) {
+    if (!cxl_doe_cdat_init(cxl_cstate, errp)) {
         goto err_free_special_ops;
     }
 
diff --git a/hw/pci-bridge/cxl_upstream.c b/hw/pci-bridge/cxl_upstream.c
index 783fa6adac19..e51221a5f334 100644
--- a/hw/pci-bridge/cxl_upstream.c
+++ b/hw/pci-bridge/cxl_upstream.c
@@ -338,8 +338,7 @@ static void cxl_usp_realize(PCIDevice *d, Error **errp)
     cxl_cstate->cdat.build_cdat_table = build_cdat_table;
     cxl_cstate->cdat.free_cdat_table = free_default_cdat_table;
     cxl_cstate->cdat.private = d;
-    cxl_doe_cdat_init(cxl_cstate, errp);
-    if (*errp) {
+    if (!cxl_doe_cdat_init(cxl_cstate, errp)) {
         goto err_cap;
     }
 
diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
index 5012fab6f763..945ee6ffd045 100644
--- a/include/hw/cxl/cxl_component.h
+++ b/include/hw/cxl/cxl_component.h
@@ -273,7 +273,7 @@ hwaddr cxl_decode_ig(int ig);
 CXLComponentState *cxl_get_hb_cstate(PCIHostState *hb);
 bool cxl_get_hb_passthrough(PCIHostState *hb);
 
-void cxl_doe_cdat_init(CXLComponentState *cxl_cstate, Error **errp);
+bool cxl_doe_cdat_init(CXLComponentState *cxl_cstate, Error **errp);
 void cxl_doe_cdat_release(CXLComponentState *cxl_cstate);
 void cxl_doe_cdat_update(CXLComponentState *cxl_cstate, Error **errp);
 
-- 
2.34.1

Re: [PATCH 3/3] hw/cxl/cxl-cdat: Make cxl_doe_cdat_init() return boolean

[Philippe Mathieu-Daudé]

On 18/4/24 12:04, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> As error.h suggested, the best practice for callee is to return
> something to indicate success / failure.
> 
> With returned boolean, there's no need to dereference @errp to check
> failure case.
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   hw/cxl/cxl-cdat.c              | 6 +++---
>   hw/mem/cxl_type3.c             | 3 +--
>   hw/pci-bridge/cxl_upstream.c   | 3 +--
>   include/hw/cxl/cxl_component.h | 2 +-
>   4 files changed, 6 insertions(+), 8 deletions(-)


> diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
> index 5012fab6f763..945ee6ffd045 100644
> --- a/include/hw/cxl/cxl_component.h
> +++ b/include/hw/cxl/cxl_component.h
> @@ -273,7 +273,7 @@ hwaddr cxl_decode_ig(int ig);
>   CXLComponentState *cxl_get_hb_cstate(PCIHostState *hb);
>   bool cxl_get_hb_passthrough(PCIHostState *hb);
>   
> -void cxl_doe_cdat_init(CXLComponentState *cxl_cstate, Error **errp);
> +bool cxl_doe_cdat_init(CXLComponentState *cxl_cstate, Error **errp);
>   void cxl_doe_cdat_release(CXLComponentState *cxl_cstate);
>   void cxl_doe_cdat_update(CXLComponentState *cxl_cstate, Error **errp);

Another candidate ;)

Re: [PATCH 0/3] hw/cxl/cxl-cdat: Make cxl_doe_cdat_init() return boolean

[Philippe Mathieu-Daudé]

On 18/4/24 12:04, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>


> ---
> Zhao Liu (3):
>    hw/cxl/cxl-cdat: Make ct3_load_cdat() return boolean
>    hw/cxl/cxl-cdat: Make ct3_build_cdat() return boolean
>    hw/cxl/cxl-cdat: Make cxl_doe_cdat_init() return boolean

Series:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 0/3] hw/cxl/cxl-cdat: Make cxl_doe_cdat_init() return boolean

[Jonathan Cameron via]

On Thu, 18 Apr 2024 14:06:39 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> On 18/4/24 12:04, Zhao Liu wrote:
> > From: Zhao Liu <zhao1.liu@intel.com>  
> 
> 
> > ---
> > Zhao Liu (3):
> >    hw/cxl/cxl-cdat: Make ct3_load_cdat() return boolean
> >    hw/cxl/cxl-cdat: Make ct3_build_cdat() return boolean
> >    hw/cxl/cxl-cdat: Make cxl_doe_cdat_init() return boolean  
> 
> Series:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH 3/3] hw/cxl/cxl-cdat: Make cxl_doe_cdat_init() return boolean

[Zhao Liu]

Hi Philippe,

On Thu, Apr 18, 2024 at 02:06:15PM +0200, Philippe Mathieu-Daudé wrote:

[snip]

> > diff --git a/include/hw/cxl/cxl_component.h b/include/hw/cxl/cxl_component.h
> > index 5012fab6f763..945ee6ffd045 100644
> > --- a/include/hw/cxl/cxl_component.h
> > +++ b/include/hw/cxl/cxl_component.h
> > @@ -273,7 +273,7 @@ hwaddr cxl_decode_ig(int ig);
> >   CXLComponentState *cxl_get_hb_cstate(PCIHostState *hb);
> >   bool cxl_get_hb_passthrough(PCIHostState *hb);
> > -void cxl_doe_cdat_init(CXLComponentState *cxl_cstate, Error **errp);
> > +bool cxl_doe_cdat_init(CXLComponentState *cxl_cstate, Error **errp);
> >   void cxl_doe_cdat_release(CXLComponentState *cxl_cstate);
> >   void cxl_doe_cdat_update(CXLComponentState *cxl_cstate, Error **errp);
> 
> Another candidate ;)
> 

I guess you mean cxl_doe_cdat_update()? ;-)

It's a special case since it has only one use case and in that case,
&error_fatal is passed as @errp. So then it doesn't need to check the
return value.

Thanks,
Zhao

Re: [PATCH 0/3] hw/cxl/cxl-cdat: Make cxl_doe_cdat_init() return boolean

[Philippe Mathieu-Daudé]

On 18/4/24 12:04, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>


> ---
> Zhao Liu (3):
>    hw/cxl/cxl-cdat: Make ct3_load_cdat() return boolean
>    hw/cxl/cxl-cdat: Make ct3_build_cdat() return boolean
>    hw/cxl/cxl-cdat: Make cxl_doe_cdat_init() return boolean

Since Jonathan Ack'ed the series, I'm queuing it via my hw-misc tree.

Re: [PATCH 0/3] hw/cxl/cxl-cdat: Make cxl_doe_cdat_init() return boolean

[Jonathan Cameron via]

On Fri, 19 Apr 2024 17:40:07 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> On 18/4/24 12:04, Zhao Liu wrote:
> > From: Zhao Liu <zhao1.liu@intel.com>  
> 
> 
> > ---
> > Zhao Liu (3):
> >    hw/cxl/cxl-cdat: Make ct3_load_cdat() return boolean
> >    hw/cxl/cxl-cdat: Make ct3_build_cdat() return boolean
> >    hw/cxl/cxl-cdat: Make cxl_doe_cdat_init() return boolean  
> 
> Since Jonathan Ack'ed the series, I'm queuing it via my hw-misc tree.
> 

Thanks,

J