| Message ID | 1451223640-2569-2-git-send-email-caoj.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On Sun, 27 Dec 2015, Cao jin wrote: > To catch the error msg. Also modify the caller > > Signed-off-by: Cao jin <caoj.fnst@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 > > >
On 01/04/2016 11:15 PM, Stefano Stabellini wrote: > On Sun, 27 Dec 2015, Cao jin wrote: >> To catch the error msg. Also modify the caller >> >> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > > This looks much better, thanks. > > [...] >> >> -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. > Thanks for reminding, I didn`t see the comment of error.h before, now I am aware why lots of people like the style you mentioned. Will fix it in next version, also the comments in other patch. [...]
On 01/04/2016 11:15 PM, Stefano Stabellini wrote: > On Sun, 27 Dec 2015, Cao jin wrote: >> To catch the error msg. Also modify the caller >> >> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > > This looks much better, thanks. > > [...] >> >> -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. > Hi stefano, I read that comment, and find something maybe new: "errp could be NULL", I think it is saying, if we are in a .realize() function, yes, *errp* maybe NULL, but reality is, here is the callee of .realize(), and we defined a local variable: Error *local_err = NULL in .realize() and passed it to all the callee, so, theoretically *errp* won`t be NULL. so the way you said above is suitable in .realize() IMHO, and I also did it in that way. comment also says: * Receive an error and pass it on to the caller: * Error *err = NULL; * foo(arg, &err); * if (err) { * handle the error... * error_propagate(errp, err); * } * where Error **errp is a parameter, by convention the last one. If I understand the last sentence well, the Error **errp in .realize() prototype is *the last one*, so we could call error_propagate(errp, err) only in .realize() The comment also says: * But when all you do with the error is pass it on, please use * foo(arg, errp); * for readability." We just pass error on in all the callees, so I guess I also did as comment suggest? How do you think? [...]
On Tue, 5 Jan 2016, Cao jin wrote: > On 01/04/2016 11:15 PM, Stefano Stabellini wrote: > > On Sun, 27 Dec 2015, Cao jin wrote: > > > To catch the error msg. Also modify the caller > > > > > > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > > > > This looks much better, thanks. > > > > > [...] > > > > > > -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. > > > > Hi stefano, > > I read that comment, and find something maybe new: > > "errp could be NULL", I think it is saying, if we are in a .realize() > function, yes, *errp* maybe NULL, but reality is, here is the callee of > .realize(), and we defined a local variable: Error *local_err = NULL in > .realize() and passed it to all the callee, so, theoretically *errp* won`t be > NULL. This is true, however I think that relying on it is error prone: in a couple of years from now somebody might change the call sequence without updating the error handling (easy to forget), causing QEMU to crash on error. I think it is safer not to rely on errp != NULL. > so the way you said above is suitable in .realize() IMHO, and I also did > it in that way. > > comment also says: > > * Receive an error and pass it on to the caller: > * Error *err = NULL; > * foo(arg, &err); > * if (err) { > * handle the error... > * error_propagate(errp, err); > * } > * where Error **errp is a parameter, by convention the last one. > > If I understand the last sentence well, the Error **errp in .realize() > prototype is *the last one*, so we could call error_propagate(errp, err) only > in .realize() > > The comment also says: > > * But when all you do with the error is pass it on, please use > * foo(arg, errp); > * for readability." > > We just pass error on in all the callees, so I guess I also did as comment > suggest? > > How do you think? I think we only need to use a local Error variable when we want to check for the returned error, in cases such as: if (*errp) { In other cases, when we are not interested in *errp, we can simply propagate the error, like you have done in your patches.
On 01/05/2016 06:40 PM, Stefano Stabellini wrote: > On Tue, 5 Jan 2016, Cao jin wrote: [...] > > This is true, however I think that relying on it is error prone: in a > couple of years from now somebody might change the call sequence without > updating the error handling (easy to forget), causing QEMU to crash on > error. I think it is safer not to rely on errp != NULL. > I see, sounds reasonable > [...] > I think we only need to use a local Error variable when we want to check > for the returned error, in cases such as: > > if (*errp) { > > In other cases, when we are not interested in *errp, we can simply > propagate the error, like you have done in your patches. > Ok, will fix it > . >
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) { 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; }
To catch the error msg. Also modify the caller Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> --- 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(-)