[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 2/5] Add Error **errp for xen_host_pci_device
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v4 2/5] Add Error **errp for xen_host_pci_device_get() |
Date: |
Fri, 8 Jan 2016 15:50:28 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 |
On 01/08/2016 01:37 AM, Cao jin wrote:
> To catch the error msg. Also modify the caller
>
> Signed-off-by: Cao jin <address@hidden>
> ---
> 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
- [Qemu-devel] [PATCH v4 0/5] Xen PCI passthru: Convert to realize(), Cao jin, 2016/01/08
- [Qemu-devel] [PATCH v4 1/5] Use qemu_strtoul instead of strtol, Cao jin, 2016/01/08
- [Qemu-devel] [PATCH v4 3/5] Add Error **errp for xen_pt_setup_vga(), Cao jin, 2016/01/08
- [Qemu-devel] [PATCH v4 2/5] Add Error **errp for xen_host_pci_device_get(), Cao jin, 2016/01/08
- Re: [Qemu-devel] [PATCH v4 2/5] Add Error **errp for xen_host_pci_device_get(),
Eric Blake <=
- [Qemu-devel] [PATCH v4 5/5] Xen PCI passthru: convert to realize(), Cao jin, 2016/01/08
- [Qemu-devel] [PATCH v4 4/5] Add Error **errp for xen_pt_config_init(), Cao jin, 2016/01/08