From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51093) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGqMv-0003Gm-EL for qemu-devel@nongnu.org; Wed, 06 Jan 2016 10:51:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aGqMr-0007hz-CK for qemu-devel@nongnu.org; Wed, 06 Jan 2016 10:51:25 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53803) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGqMr-0007ht-4e for qemu-devel@nongnu.org; Wed, 06 Jan 2016 10:51:21 -0500 References: <1452047951-17429-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1452047951-17429-2-git-send-email-caoj.fnst@cn.fujitsu.com> From: Eric Blake Message-ID: <568D37F0.6080101@redhat.com> Date: Wed, 6 Jan 2016 08:51:12 -0700 MIME-Version: 1.0 In-Reply-To: <1452047951-17429-2-git-send-email-caoj.fnst@cn.fujitsu.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="DqiAMMTsHK8FNw2VBHTFkhiiAhdl5jQX4" Subject: Re: [Qemu-devel] [PATCH v3 1/4] Add Error **errp for xen_host_pci_device_get() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cao jin , qemu-devel@nongnu.org Cc: stefano.stabellini@eu.citrix.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --DqiAMMTsHK8FNw2VBHTFkhiiAhdl5jQX4 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 01/05/2016 07:39 PM, Cao jin wrote: > To catch the error msg. Also modify the caller >=20 > Signed-off-by: Cao jin > --- > 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(-) >=20 > 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 XenHostPCIDe= vice *d, > =20 > /* 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 **err= p) > { > int i, rc, fd; > char path[PATH_MAX]; > @@ -60,23 +60,24 @@ static int xen_host_pci_get_resource(XenHostPCIDevi= ce *d) > =20 > rc =3D 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 =3D open(path, O_RDONLY); > if (fd =3D=3D -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(XenHostPCIDe= vice *d) > d->rom.bus_flags =3D flags & IORESOURCE_BITS; > } > } > + > if (i !=3D PCI_NUM_REGIONS) { > /* Invalid format or input to short */ > - rc =3D -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(XenHostPCIDevic= e *d, const char *name, > =20 > rc =3D xen_host_pci_sysfs_path(d, name, path, sizeof (path)); > if (rc) { > - return rc; > + error_setg_errno(errp, errno, "snprintf err"); > + return; > } > + > fd =3D open(path, O_RDONLY); > if (fd =3D=3D -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 =3D read(fd, &buf, sizeof (buf) - 1); > if (rc < 0 && errno !=3D EINTR) { > - rc =3D -errno; > + error_setg_errno(errp, errno, "read err"); > goto out; > } > } while (rc < 0); > + > buf[rc] =3D 0; > value =3D strtol(buf, &endptr, base); > if (endptr =3D=3D buf || *endptr !=3D '\n') { > - rc =3D -1; > + error_setg(errp, "format invalid: %s", buf); > } else if ((value =3D=3D LONG_MIN || value =3D=3D LONG_MAX) && err= no =3D=3D ERANGE) { > - rc =3D -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; > =20 > rc =3D 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 =3D 0; > + Error *local_err =3D NULL; These days, naming the local variable 'err' is more common than 'local_er= r'. > @@ -774,11 +775,12 @@ static int xen_pt_initfn(PCIDevice *d) > s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function= , > s->dev.devfn); > =20 > - rc =3D xen_host_pci_device_get(&s->real_device, > - s->hostaddr.domain, s->hostaddr.bus, > - s->hostaddr.slot, s->hostaddr.functio= n); > - 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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --DqiAMMTsHK8FNw2VBHTFkhiiAhdl5jQX4 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWjTfwAAoJEKeha0olJ0Nqov4H/0OGR7FkCDFozYMH/7I1dFM7 ESt7Gtf5Ima0ipVc0bNiWVvFiufvb2hFFaLyqd3SJWeiwKsVqDFuSPyrfEnDJwgc VVTxKMADiNG9eW2W/n6BM1vir5iqTw3Xr8rcXpjDLvVI4AZYs//6IZzRlFFtmna+ mD2UMVoNcGw/ibiMaENdFC6H9X2/cOs3q6NXLEz30NzZzLM3VxjcmnTDm+M2Bcy7 eD4HwV4mYHzGr2feo3SsUhEBGcwopNdqs7ipdU3CDQmjcSkLTUEDkwbRcJb4QOaO tFogk1EuNohcwTTRUyvkzwW3IyIK9b1bSoLfsnTsxseUO1k34f/UjDkvLDQ2xMg= =3phm -----END PGP SIGNATURE----- --DqiAMMTsHK8FNw2VBHTFkhiiAhdl5jQX4--