Re: [Qemu-devel] [PATCH v2 3/4] Add Error **errp for xen_pt_config_init()

[Stefano Stabellini]

On Sun, 27 Dec 2015, Cao jin wrote:
> To catch the error msg. Also modify the caller
> 
> Signed-off-by: Cao jin <address@hidden>
> ---
>  hw/xen/xen_pt.c             |  7 ++++---
>  hw/xen/xen_pt.h             |  2 +-
>  hw/xen/xen_pt_config_init.c | 48 
> ++++++++++++++++++++++-----------------------
>  3 files changed, 29 insertions(+), 28 deletions(-)
> 
> 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..096f204 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,11 +1993,9 @@ 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;
>  
> @@ -2039,11 +2040,11 @@ 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(errp, "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);
>                  xen_pt_config_delete(s);
> -                return rc;
> +                return;
>              }
>          }
>  
> @@ -2051,24 +2052,23 @@ 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, errp);
> +                    if (*errp) {

errp could theoretically be NULL


> +                        error_append_hint(errp, "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));
>                          xen_pt_config_delete(s);
> -                        return rc;
> +                        return;
>                      }
>                  }
>              }
>          }
>      }
> -
> -    return 0;
>  }
>  
>  /* delete all emulate register */
> -- 
> 2.1.0
> 
> 
>