diff mbox

[v3,3/4] Add Error **errp for xen_pt_config_init()

Message ID 1452047951-17429-4-git-send-email-caoj.fnst@cn.fujitsu.com
State New
Headers show

Commit Message

Cao jin Jan. 6, 2016, 2:39 a.m. UTC 
To catch the error msg. Also modify the caller

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/xen/xen_pt.c             |  7 ++++---
 hw/xen/xen_pt.h             |  2 +-
 hw/xen/xen_pt_config_init.c | 51 ++++++++++++++++++++++++---------------------
 3 files changed, 32 insertions(+), 28 deletions(-)

Comments

Eric Blake Jan. 6, 2016, 4:02 p.m. UTC | #1 
On 01/05/2016 07:39 PM, Cao jin wrote:
> To catch the error msg. Also modify the caller
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/xen/xen_pt.c             |  7 ++++---
>  hw/xen/xen_pt.h             |  2 +-
>  hw/xen/xen_pt_config_init.c | 51 ++++++++++++++++++++++++---------------------
>  3 files changed, 32 insertions(+), 28 deletions(-)
> 

> +++ b/hw/xen/xen_pt_config_init.c
> @@ -1899,8 +1899,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)

Indentation is now off.


> @@ -1967,10 +1970,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);

Drop the trailing !.  Also, while touching this, it's better to have a
space before ( in English.


> +void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp)
>  {
>      int i, rc;
> +    Error *local_err = NULL;

Same comments as earlier in the series about using the shorter 'err'
instead of 'local_err'.

>  
>      QLIST_INIT(&s->reg_grps);
>  
> @@ -2039,11 +2041,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(&local_err, "Failed to initialize %d/%ld, type=0x%x,"
> +                           " rc:%d", i, ARRAY_SIZE(xen_pt_emu_reg_grps),

This maps ARRAY_SIZE() (which is size_t) to %ld, which can fail to
compile on 32-bit platforms (where size_t is not necessarily long).  Fix
it to %zd while touching it.
Cao jin Jan. 7, 2016, 8:12 a.m. UTC | #2 
On 01/07/2016 12:02 AM, Eric Blake wrote:
> On 01/05/2016 07:39 PM, Cao jin wrote:
[...]
>> +static void xen_pt_config_reg_init(XenPCIPassthroughState *s,
>> +                                  XenPTRegGroup *reg_grp, XenPTRegInfo *reg,
>> +                                  Error **errp)
>
> Indentation is now off.
>

Sharp eyes;)

>> @@ -1967,10 +1970,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);
>
> Drop the trailing !.  Also, while touching this, it's better to have a
> space before ( in English.
>

Ok

>
>> +void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp)
>>   {
>>       int i, rc;
>> +    Error *local_err = NULL;
>
> Same comments as earlier in the series about using the shorter 'err'
> instead of 'local_err'.
>
>>
>>       QLIST_INIT(&s->reg_grps);
>>
>> @@ -2039,11 +2041,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(&local_err, "Failed to initialize %d/%ld, type=0x%x,"
>> +                           " rc:%d", i, ARRAY_SIZE(xen_pt_emu_reg_grps),
>
> This maps ARRAY_SIZE() (which is size_t) to %ld, which can fail to
> compile on 32-bit platforms (where size_t is not necessarily long).  Fix
> it to %zd while touching it.
>

a question:
1. Is %zu more suitable for size_t? since size_t is unsigned integer.

and a personal question after digging into size_t:
2. Does the size of size_t always equal to the word length[*] of computer

[*] https://en.wikipedia.org/wiki/Word_%28computer_architecture%29
Eric Blake Jan. 7, 2016, 4:51 p.m. UTC | #3 
On 01/07/2016 01:12 AM, Cao jin wrote:

>>>               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(&local_err, "Failed to initialize %d/%ld,
>>> type=0x%x,"
>>> +                           " rc:%d", i,
>>> ARRAY_SIZE(xen_pt_emu_reg_grps),
>>
>> This maps ARRAY_SIZE() (which is size_t) to %ld, which can fail to
>> compile on 32-bit platforms (where size_t is not necessarily long).  Fix
>> it to %zd while touching it.
>>
> 
> a question:
> 1. Is %zu more suitable for size_t? since size_t is unsigned integer.

Yes, you're right on that one.

> 
> and a personal question after digging into size_t:
> 2. Does the size of size_t always equal to the word length[*] of computer

No.  It equals the maximum size the program can use.  But the x32 ABI
project is a good example of a 32-bit size_t while still taking full
advantage of the 64-bit word size registers, in the name of memory
efficiencies.  See https://en.wikipedia.org/wiki/X32_ABI.
Cao jin Jan. 8, 2016, 8:41 a.m. UTC | #4 
On 01/08/2016 12:51 AM, Eric Blake wrote:
> On 01/07/2016 01:12 AM, Cao jin wrote:
>
>>>>                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(&local_err, "Failed to initialize %d/%ld,
>>>> type=0x%x,"
>>>> +                           " rc:%d", i,
>>>> ARRAY_SIZE(xen_pt_emu_reg_grps),
>>>
>>> This maps ARRAY_SIZE() (which is size_t) to %ld, which can fail to
>>> compile on 32-bit platforms (where size_t is not necessarily long).  Fix
>>> it to %zd while touching it.
>>>
>>
>> a question:
>> 1. Is %zu more suitable for size_t? since size_t is unsigned integer.
>
> Yes, you're right on that one.
>
>>
>> and a personal question after digging into size_t:
>> 2. Does the size of size_t always equal to the word length[*] of computer
>
> No.  It equals the maximum size the program can use.  But the x32 ABI
> project is a good example of a 32-bit size_t while still taking full
> advantage of the 64-bit word size registers, in the name of memory
> efficiencies.  See https://en.wikipedia.org/wiki/X32_ABI.
>

Thanks very much. Have send v4 version, hope I don`t miss any comment:)
diff mbox

Patch

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index fbce55c..3787c26 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -824,9 +824,10 @@  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, &local_err);
+    if (local_err) {
+        error_append_hint(&local_err, "PCI Config space initialisation failed");
+        rc = -1;
         goto err_out;
     }
 
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index dc74d3e..466525f 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -228,7 +228,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 3d8686d..72b10dd 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1899,8 +1899,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;
@@ -1919,12 +1920,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;
@@ -1942,7 +1944,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
@@ -1967,10 +1970,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
@@ -1990,13 +1993,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 *local_err = NULL;
 
     QLIST_INIT(&s->reg_grps);
 
@@ -2039,11 +2041,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(&local_err, "Failed to initialize %d/%ld, 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, local_err);
                 xen_pt_config_delete(s);
-                return rc;
+                return;
             }
         }
 
@@ -2051,24 +2054,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, &local_err);
+                    if (local_err) {
+                        error_append_hint(&local_err, "Failed to initialize %d/%ld"
+                                " reg 0x%x in grp_type=0x%x (%d/%ld)",
+                                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, local_err);
                         xen_pt_config_delete(s);
-                        return rc;
+                        return;
                     }
                 }
             }
         }
     }
-
-    return 0;
 }
 
 /* delete all emulate register */