Re: [Qemu-devel] [PATCH v4 2/5] Add Error **errp for xen_host_pci_device_get()

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4844 bytes --]

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.fnst@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]