On 01/05/2016 07:39 PM, 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 | 106
> +++++++++++++++++++++++++------------------
> hw/xen/xen-host-pci-device.h | 5 +-
> hw/xen/xen_pt.c | 12 +++--
> 3 files changed, 71 insertions(+), 52 deletions(-)
>
> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> index 7d8a023..952cae0 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");
Are you sure that errno is relevant? And "snprintf err" doesn't seem to
be the correct message, as there is no snprintf in the line above.
> + 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);
Please use error_setg_file_open() for reporting open() failures.
> @@ -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);
s/to/too/ (and might as well fix the same typo in the comment while at it)
> @@ -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);
Same comments as above.
> + 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");
This is pre-existing invalid use of strtol (the value of errno is not
guaranteed to be ERANGE on overflow; and the only correct way to safely
check errno after strtol() is to first prime it to 0 prior to calling
strtol). Better would be to use qemu_strtol() (preferably as a separate
patch), so that you don't even have to worry about using strtol()
incorrectly.
> +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));
May want to delete the space before ( while touching the code in this
vicinity.
> if (rc) {
> - return rc;
> + error_setg_errno(errp, errno, "snprintf err");
Another suspect message.
> +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;
> + Error *local_err = NULL;
These days, naming the local variable 'err' is more common than 'local_err'.
> @@ -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");
Leaks local_err.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
