On Sun, 27 Dec 2015, Cao jin wrote:
> To catch the error msg. Also modify the caller
>
> Signed-off-by: Cao jin <caoj.f...@cn.fujitsu.com>
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
>
>
>
