diff mbox

[PULL,10/11] Add Error **errp for xen_pt_config_init()

Message ID 1453395690-32660-10-git-send-email-stefano.stabellini@eu.citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Stefano Stabellini Jan. 21, 2016, 5:01 p.m. UTC 
From: Cao jin <caoj.fnst@cn.fujitsu.com>

To catch the error message. Also modify the caller

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/xen/xen_pt.c             |    8 ++++---
 hw/xen/xen_pt.h             |    2 +-
 hw/xen/xen_pt_config_init.c |   51 +++++++++++++++++++++++--------------------
 3 files changed, 33 insertions(+), 28 deletions(-)

Comments

Paolo Bonzini Jan. 22, 2016, 11:21 a.m. UTC | #1 
On 21/01/2016 18:01, Stefano Stabellini wrote:
> -                        XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld reg 0x%x in grp_type=0x%x (%d/%ld), rc=%d\n",
> -                                   j, ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs),
> -                                   regs->offset, xen_pt_emu_reg_grps[i].grp_type,
> -                                   i, ARRAY_SIZE(xen_pt_emu_reg_grps), rc);
> +                    xen_pt_config_reg_init(s, reg_grp_entry, regs, &err);
> +                    if (err) {
> +                        error_append_hint(&err, "Failed to initialize %d/%zu"
> +                                " reg 0x%x in grp_type = 0x%x (%d/%zu)",
> +                                j, ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs),

Coverity noticed a preexisting problem here.  emu_regs is a pointer,
thus ARRAY_SIZE doesn't return what you expect.

Paolo

> +                                regs->offset, xen_pt_emu_reg_grps[i].grp_type,
> +                                i, ARRAY_SIZE(xen_pt_emu_reg_grps));
> +                        error_propagate(errp, err);
Cao jin Jan. 23, 2016, 12:23 p.m. UTC | #2 
On 01/22/2016 07:21 PM, Paolo Bonzini wrote:
>
>
> On 21/01/2016 18:01, Stefano Stabellini wrote:
>> -                        XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld reg 0x%x in grp_type=0x%x (%d/%ld), rc=%d\n",
>> -                                   j, ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs),
>> -                                   regs->offset, xen_pt_emu_reg_grps[i].grp_type,
>> -                                   i, ARRAY_SIZE(xen_pt_emu_reg_grps), rc);
>> +                    xen_pt_config_reg_init(s, reg_grp_entry, regs, &err);
>> +                    if (err) {
>> +                        error_append_hint(&err, "Failed to initialize %d/%zu"
>> +                                " reg 0x%x in grp_type = 0x%x (%d/%zu)",
>> +                                j, ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs),
>
> Coverity noticed a preexisting problem here.  emu_regs is a pointer,
> thus ARRAY_SIZE doesn't return what you expect.
>
Hi stefano,

Seems ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs) is not important err 
message to regular users, and I guess it still can help developer to 
debug even without it. So, do you think it is ok to remove it? Or any 
better idea?

> Paolo
>
Stefano Stabellini Jan. 25, 2016, 10:53 a.m. UTC | #3 
On Sat, 23 Jan 2016, Cao jin wrote:
> On 01/22/2016 07:21 PM, Paolo Bonzini wrote:
> > 
> > 
> > On 21/01/2016 18:01, Stefano Stabellini wrote:
> > > -                        XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld
> > > reg 0x%x in grp_type=0x%x (%d/%ld), rc=%d\n",
> > > -                                   j,
> > > ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs),
> > > -                                   regs->offset,
> > > xen_pt_emu_reg_grps[i].grp_type,
> > > -                                   i, ARRAY_SIZE(xen_pt_emu_reg_grps),
> > > rc);
> > > +                    xen_pt_config_reg_init(s, reg_grp_entry, regs, &err);
> > > +                    if (err) {
> > > +                        error_append_hint(&err, "Failed to initialize
> > > %d/%zu"
> > > +                                " reg 0x%x in grp_type = 0x%x (%d/%zu)",
> > > +                                j,
> > > ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs),
> > 
> > Coverity noticed a preexisting problem here.  emu_regs is a pointer,
> > thus ARRAY_SIZE doesn't return what you expect.
> > 
> Hi stefano,
> 
> Seems ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs) is not important err message
> to regular users, and I guess it still can help developer to debug even
> without it. So, do you think it is ok to remove it? Or any better idea?

Yes, it is OK to remove it.
diff mbox

Patch

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 07bfcec..9eef3df 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -825,9 +825,11 @@  static int xen_pt_initfn(PCIDevice *d)
     xen_pt_register_regions(s, &cmd);
 
     /* reinitialize each config register to be emulated */
-    rc = xen_pt_config_init(s);
-    if (rc) {
-        XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
+    xen_pt_config_init(s, &err);
+    if (err) {
+        error_append_hint(&err, "PCI Config space initialisation failed");
+        error_report_err(err);
+        rc = -1;
         goto err_out;
     }
 
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 26f74f8..c2f8e1f 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -230,7 +230,7 @@  struct XenPCIPassthroughState {
     bool listener_set;
 };
 
-int xen_pt_config_init(XenPCIPassthroughState *s);
+void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp);
 void xen_pt_config_delete(XenPCIPassthroughState *s);
 XenPTRegGroup *xen_pt_find_reg_grp(XenPCIPassthroughState *s, uint32_t address);
 XenPTReg *xen_pt_find_reg(XenPTRegGroup *reg_grp, uint32_t address);
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 185a698..81c6721 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1887,8 +1887,9 @@  static uint8_t find_cap_offset(XenPCIPassthroughState *s, uint8_t cap)
     return 0;
 }
 
-static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
-                                  XenPTRegGroup *reg_grp, XenPTRegInfo *reg)
+static void xen_pt_config_reg_init(XenPCIPassthroughState *s,
+                                   XenPTRegGroup *reg_grp, XenPTRegInfo *reg,
+                                   Error **errp)
 {
     XenPTReg *reg_entry;
     uint32_t data = 0;
@@ -1907,12 +1908,13 @@  static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
                        reg_grp->base_offset + reg->offset, &data);
         if (rc < 0) {
             g_free(reg_entry);
-            return rc;
+            error_setg(errp, "Init emulate register fail");
+            return;
         }
         if (data == XEN_PT_INVALID_REG) {
             /* free unused BAR register entry */
             g_free(reg_entry);
-            return 0;
+            return;
         }
         /* Sync up the data to dev.config */
         offset = reg_grp->base_offset + reg->offset;
@@ -1930,7 +1932,8 @@  static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
         if (rc) {
             /* Serious issues when we cannot read the host values! */
             g_free(reg_entry);
-            return rc;
+            error_setg(errp, "Cannot read host values");
+            return;
         }
         /* Set bits in emu_mask are the ones we emulate. The dev.config shall
          * contain the emulated view of the guest - therefore we flip the mask
@@ -1955,10 +1958,10 @@  static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
             val = data;
 
         if (val & ~size_mask) {
-            XEN_PT_ERR(&s->dev,"Offset 0x%04x:0x%04x expands past register size(%d)!\n",
-                       offset, val, reg->size);
+            error_setg(errp, "Offset 0x%04x:0x%04x expands past"
+                    " register size (%d)", offset, val, reg->size);
             g_free(reg_entry);
-            return -ENXIO;
+            return;
         }
         /* This could be just pci_set_long as we don't modify the bits
          * past reg->size, but in case this routine is run in parallel or the
@@ -1978,13 +1981,12 @@  static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
     }
     /* list add register entry */
     QLIST_INSERT_HEAD(&reg_grp->reg_tbl_list, reg_entry, entries);
-
-    return 0;
 }
 
-int xen_pt_config_init(XenPCIPassthroughState *s)
+void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp)
 {
     int i, rc;
+    Error *err = NULL;
 
     QLIST_INIT(&s->reg_grps);
 
@@ -2027,11 +2029,12 @@  int xen_pt_config_init(XenPCIPassthroughState *s)
                                                   reg_grp_offset,
                                                   &reg_grp_entry->size);
             if (rc < 0) {
-                XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld, type=0x%x, rc:%d\n",
-                           i, ARRAY_SIZE(xen_pt_emu_reg_grps),
+                error_setg(&err, "Failed to initialize %d/%zu, type = 0x%x,"
+                           " rc: %d", i, ARRAY_SIZE(xen_pt_emu_reg_grps),
                            xen_pt_emu_reg_grps[i].grp_type, rc);
+                error_propagate(errp, err);
                 xen_pt_config_delete(s);
-                return rc;
+                return;
             }
         }
 
@@ -2039,24 +2042,24 @@  int xen_pt_config_init(XenPCIPassthroughState *s)
             if (xen_pt_emu_reg_grps[i].emu_regs) {
                 int j = 0;
                 XenPTRegInfo *regs = xen_pt_emu_reg_grps[i].emu_regs;
+
                 /* initialize capability register */
                 for (j = 0; regs->size != 0; j++, regs++) {
-                    /* initialize capability register */
-                    rc = xen_pt_config_reg_init(s, reg_grp_entry, regs);
-                    if (rc < 0) {
-                        XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld reg 0x%x in grp_type=0x%x (%d/%ld), rc=%d\n",
-                                   j, ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs),
-                                   regs->offset, xen_pt_emu_reg_grps[i].grp_type,
-                                   i, ARRAY_SIZE(xen_pt_emu_reg_grps), rc);
+                    xen_pt_config_reg_init(s, reg_grp_entry, regs, &err);
+                    if (err) {
+                        error_append_hint(&err, "Failed to initialize %d/%zu"
+                                " reg 0x%x in grp_type = 0x%x (%d/%zu)",
+                                j, ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs),
+                                regs->offset, xen_pt_emu_reg_grps[i].grp_type,
+                                i, ARRAY_SIZE(xen_pt_emu_reg_grps));
+                        error_propagate(errp, err);
                         xen_pt_config_delete(s);
-                        return rc;
+                        return;
                     }
                 }
             }
         }
     }
-
-    return 0;
 }
 
 /* delete all emulate register */