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

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

This looks much better, thanks.


>  hw/xen/xen-host-pci-device.c | 102 
> ++++++++++++++++++++++++-------------------
>  hw/xen/xen-host-pci-device.h |   5 ++-
>  hw/xen/xen_pt.c              |  12 ++---
>  3 files changed, 67 insertions(+), 52 deletions(-)
> 
> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> index 7d8a023..3d22095 100644
> --- a/hw/xen/xen-host-pci-device.c
> +++ b/hw/xen/xen-host-pci-device.c
> @@ -49,7 +49,7 @@ static int xen_host_pci_sysfs_path(const XenHostPCIDevice 
> *d,
>  
>  /* This size should be enough to read the first 7 lines of a resource file */
>  #define XEN_HOST_PCI_RESOURCE_BUFFER_SIZE 400
> -static int xen_host_pci_get_resource(XenHostPCIDevice *d)
> +static void xen_host_pci_get_resource(XenHostPCIDevice *d, Error **errp)
>  {
>      int i, rc, fd;
>      char path[PATH_MAX];
> @@ -60,23 +60,24 @@ static int xen_host_pci_get_resource(XenHostPCIDevice *d)
>  
>      rc = xen_host_pci_sysfs_path(d, "resource", path, sizeof (path));
>      if (rc) {
> -        return rc;
> +        error_setg_errno(errp, errno, "snprintf err");
> +        return;
>      }
> +
>      fd = open(path, O_RDONLY);
>      if (fd == -1) {
> -        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, 
> strerror(errno));
> -        return -errno;
> +        error_setg_errno(errp, errno, "open %s err", path);
> +        return;
>      }
>  
>      do {
>          rc = read(fd, &buf, sizeof (buf) - 1);
>          if (rc < 0 && errno != EINTR) {
> -            rc = -errno;
> +            error_setg_errno(errp, errno, "read err");
>              goto out;
>          }
>      } while (rc < 0);
>      buf[rc] = 0;
> -    rc = 0;
>  
>      s = buf;
>      for (i = 0; i < PCI_NUM_REGIONS; i++) {
> @@ -129,20 +130,20 @@ static int xen_host_pci_get_resource(XenHostPCIDevice 
> *d)
>              d->rom.bus_flags = flags & IORESOURCE_BITS;
>          }
>      }
> +
>      if (i != PCI_NUM_REGIONS) {
>          /* Invalid format or input to short */
> -        rc = -ENODEV;
> +        error_setg(errp, "Invalid format or input to short: %s", buf);
>      }
>  
>  out:
>      close(fd);
> -    return rc;
>  }
>  
>  /* This size should be enough to read a long from a file */
>  #define XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE 22
> -static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
> -                                  unsigned int *pvalue, int base)
> +static void xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
> +                                  unsigned int *pvalue, int base, Error 
> **errp)
>  {
>      char path[PATH_MAX];
>      char buf[XEN_HOST_PCI_GET_VALUE_BUFFER_SIZE];
> @@ -152,47 +153,52 @@ static int xen_host_pci_get_value(XenHostPCIDevice *d, 
> const char *name,
>  
>      rc = xen_host_pci_sysfs_path(d, name, path, sizeof (path));
>      if (rc) {
> -        return rc;
> +        error_setg_errno(errp, errno, "snprintf err");
> +        return;
>      }
> +
>      fd = open(path, O_RDONLY);
>      if (fd == -1) {
> -        XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path, 
> strerror(errno));
> -        return -errno;
> +        error_setg_errno(errp, errno, "open %s err", path);
> +        return;
>      }
> +
>      do {
>          rc = read(fd, &buf, sizeof (buf) - 1);
>          if (rc < 0 && errno != EINTR) {
> -            rc = -errno;
> +            error_setg_errno(errp, errno, "read err");
>              goto out;
>          }
>      } while (rc < 0);
> +
>      buf[rc] = 0;
>      value = strtol(buf, &endptr, base);
>      if (endptr == buf || *endptr != '\n') {
> -        rc = -1;
> +        error_setg(errp, "format invalid: %s", buf);
>      } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
> -        rc = -errno;
> +        error_setg_errno(errp, errno, "strtol err");
>      } else {
> -        rc = 0;
>          *pvalue = value;
>      }
> +
>  out:
>      close(fd);
> -    return rc;
>  }
>  
> -static inline int xen_host_pci_get_hex_value(XenHostPCIDevice *d,
> +static inline void xen_host_pci_get_hex_value(XenHostPCIDevice *d,
>                                               const char *name,
> -                                             unsigned int *pvalue)
> +                                             unsigned int *pvalue,
> +                                             Error **errp)
>  {
> -    return xen_host_pci_get_value(d, name, pvalue, 16);
> +    xen_host_pci_get_value(d, name, pvalue, 16, errp);
>  }
>  
> -static inline int xen_host_pci_get_dec_value(XenHostPCIDevice *d,
> +static inline void xen_host_pci_get_dec_value(XenHostPCIDevice *d,
>                                               const char *name,
> -                                             unsigned int *pvalue)
> +                                             unsigned int *pvalue,
> +                                             Error **errp)
>  {
> -    return xen_host_pci_get_value(d, name, pvalue, 10);
> +    xen_host_pci_get_value(d, name, pvalue, 10, errp);
>  }
>  
>  static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
> @@ -206,20 +212,21 @@ static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice 
> *d)
>      return !stat(path, &buf);
>  }
>  
> -static int xen_host_pci_config_open(XenHostPCIDevice *d)
> +static void xen_host_pci_config_open(XenHostPCIDevice *d, Error **errp)
>  {
>      char path[PATH_MAX];
>      int rc;
>  
>      rc = xen_host_pci_sysfs_path(d, "config", path, sizeof (path));
>      if (rc) {
> -        return rc;
> +        error_setg_errno(errp, errno, "snprintf err");
> +        return;
>      }
> +
>      d->config_fd = open(path, O_RDWR);
>      if (d->config_fd < 0) {
> -        return -errno;
> +        error_setg_errno(errp, errno, "open %s err", path);
>      }
> -    return 0;
>  }
>  
>  static int xen_host_pci_config_read(XenHostPCIDevice *d,
> @@ -341,11 +348,11 @@ int xen_host_pci_find_ext_cap_offset(XenHostPCIDevice 
> *d, uint32_t cap)
>      return -1;
>  }
>  
> -int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
> -                            uint8_t bus, uint8_t dev, uint8_t func)
> +void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
> +                            uint8_t bus, uint8_t dev, uint8_t func,
> +                            Error **errp)
>  {
>      unsigned int v;
> -    int rc = 0;
>  
>      d->config_fd = -1;
>      d->domain = domain;
> @@ -353,43 +360,48 @@ int xen_host_pci_device_get(XenHostPCIDevice *d, 
> uint16_t domain,
>      d->dev = dev;
>      d->func = func;
>  
> -    rc = xen_host_pci_config_open(d);
> -    if (rc) {
> +    xen_host_pci_config_open(d, errp);
> +    if (*errp) {

I think that errp could be NULL, therefore the right way to do this is:

    Error *err = NULL;
    foo(arg, &err);
    if (err) {
        handle the error...
        error_propagate(errp, err);
    }

see the comment at the beginning of include/qapi/error.h.


>          goto error;
>      }
> -    rc = xen_host_pci_get_resource(d);
> -    if (rc) {
> +
> +    xen_host_pci_get_resource(d, errp);
> +    if (*errp) {
>          goto error;
>      }
> -    rc = xen_host_pci_get_hex_value(d, "vendor", &v);
> -    if (rc) {
> +
> +    xen_host_pci_get_hex_value(d, "vendor", &v, errp);
> +    if (*errp) {
>          goto error;
>      }
>      d->vendor_id = v;
> -    rc = xen_host_pci_get_hex_value(d, "device", &v);
> -    if (rc) {
> +
> +    xen_host_pci_get_hex_value(d, "device", &v, errp);
> +    if (*errp) {
>          goto error;
>      }
>      d->device_id = v;
> -    rc = xen_host_pci_get_dec_value(d, "irq", &v);
> -    if (rc) {
> +
> +    xen_host_pci_get_dec_value(d, "irq", &v, errp);
> +    if (*errp) {
>          goto error;
>      }
>      d->irq = v;
> -    rc = xen_host_pci_get_hex_value(d, "class", &v);
> -    if (rc) {
> +
> +    xen_host_pci_get_hex_value(d, "class", &v, errp);
> +    if (*errp) {
>          goto error;
>      }
>      d->class_code = v;
> +
>      d->is_virtfn = xen_host_pci_dev_is_virtfn(d);
>  
> -    return 0;
> +    return;
>  error:
>      if (d->config_fd >= 0) {
>          close(d->config_fd);
>          d->config_fd = -1;
>      }
> -    return rc;
>  }
>  
>  bool xen_host_pci_device_closed(XenHostPCIDevice *d)
> diff --git a/hw/xen/xen-host-pci-device.h b/hw/xen/xen-host-pci-device.h
> index 3d44e04..42b9138 100644
> --- a/hw/xen/xen-host-pci-device.h
> +++ b/hw/xen/xen-host-pci-device.h
> @@ -36,8 +36,9 @@ typedef struct XenHostPCIDevice {
>      int config_fd;
>  } XenHostPCIDevice;
>  
> -int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
> -                            uint8_t bus, uint8_t dev, uint8_t func);
> +void xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
> +                            uint8_t bus, uint8_t dev, uint8_t func,
> +                            Error **errp);
>  void xen_host_pci_device_put(XenHostPCIDevice *pci_dev);
>  bool xen_host_pci_device_closed(XenHostPCIDevice *d);
>  
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index aa96288..1bd4109 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -767,6 +767,7 @@ static int xen_pt_initfn(PCIDevice *d)
>      uint8_t machine_irq = 0, scratch;
>      uint16_t cmd = 0;
>      int pirq = XEN_PT_UNASSIGNED_PIRQ;
> +    Error *local_err = NULL;
>  
>      /* register real device */
>      XEN_PT_LOG(d, "Assigning real physical device %02x:%02x.%d"
> @@ -774,11 +775,12 @@ static int xen_pt_initfn(PCIDevice *d)
>                 s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function,
>                 s->dev.devfn);
>  
> -    rc = xen_host_pci_device_get(&s->real_device,
> -                                 s->hostaddr.domain, s->hostaddr.bus,
> -                                 s->hostaddr.slot, s->hostaddr.function);
> -    if (rc) {
> -        XEN_PT_ERR(d, "Failed to \"open\" the real pci device. rc: %i\n", 
> rc);
> +    xen_host_pci_device_get(&s->real_device,
> +                            s->hostaddr.domain, s->hostaddr.bus,
> +                            s->hostaddr.slot, s->hostaddr.function,
> +                            &local_err);
> +    if (local_err) {
> +        XEN_PT_ERR(d, "Failed to \"open\" the real pci device.\n");
>          return -1;
>      }
>  
> -- 
> 2.1.0
> 
> 
>