On 01/08/2016 01:37 AM, Cao jin wrote: > To catch the error msg. Also modify the caller > > Signed-off-by: Cao jin <caoj.f...@cn.fujitsu.com> > --- > hw/xen/xen-host-pci-device.c | 134 > ++++++++++++++++++++++--------------------- > hw/xen/xen-host-pci-device.h | 5 +- > hw/xen/xen_pt.c | 13 +++-- > 3 files changed, 81 insertions(+), 71 deletions(-) >
> @@ -40,16 +40,16 @@ static int xen_host_pci_sysfs_path(const XenHostPCIDevice > *d, > d->domain, d->bus, d->dev, d->func, name); > > if (rc >= size || rc < 0) { > - /* The output is truncated, or some other error was encountered */ > - return -ENODEV; > + /* The output is truncated, or some other error was encountered. > + * Assert here since user can do nothing in case of failure */ > + assert(0); > } Might be shorter to drop the 'if' block, and just write: assert(rc >= 0 && rc < size); where you then don't need a comment, because the body of the assert() is then more specific on the caller's responsibility for passing in a decent size argument. > buf[rc] = 0; > rc = qemu_strtoul(buf, &endptr, base, &value); Do you still need a local 'value' variable, or can you just reuse pvalue here? > if (!rc) { > *pvalue = value; > + } else if (rc == -EINVAL) { > + error_setg(errp, "strtoul: Invalid argument"); > + } else { > + error_setg_errno(errp, errno, "strtoul err"); Still not quite right - you are not guaranteed that 'errno' is sane after qemu_strtoul(), only that -rc is sane. And feels repetitive. Better might be: rc = qemu_strtoul(buf, &endptr, base, pvalue); if (rc) { error_setg_errno(errp, -rc, "failed to parse value '%s'", buf); } > -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) Indentation is off. > { > - 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) and again. > -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) and again. > +++ 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); and again > @@ -774,11 +775,13 @@ 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, > + &err); > + if (err) { > + error_append_hint(&err, "Failed to \"open\" the real pci device"); Markus may have an opinion on whether his new error_prepend code is a better fit (error_append_hint lists _after_ the original failure, but it sounds like you want "failed to open the real pci device: low level details"). But looks like you're getting closer. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature