From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42836) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHfrk-0006Jr-2v for qemu-devel@nongnu.org; Fri, 08 Jan 2016 17:50:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aHfrf-0008VO-QA for qemu-devel@nongnu.org; Fri, 08 Jan 2016 17:50:40 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36258) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHfrf-0008V6-IO for qemu-devel@nongnu.org; Fri, 08 Jan 2016 17:50:35 -0500 References: <1452242274-8345-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1452242274-8345-3-git-send-email-caoj.fnst@cn.fujitsu.com> From: Eric Blake Message-ID: <56903D34.2020204@redhat.com> Date: Fri, 8 Jan 2016 15:50:28 -0700 MIME-Version: 1.0 In-Reply-To: <1452242274-8345-3-git-send-email-caoj.fnst@cn.fujitsu.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="DshQ1KnX8QuHOm3tShgncRfcAEKpwtxrA" Subject: Re: [Qemu-devel] [PATCH v4 2/5] 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: Markus Armbruster , stefano.stabellini@eu.citrix.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --DshQ1KnX8QuHOm3tShgncRfcAEKpwtxrA Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 01/08/2016 01:37 AM, 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 | 134 ++++++++++++++++++++++-------------= -------- > hw/xen/xen-host-pci-device.h | 5 +- > hw/xen/xen_pt.c | 13 +++-- > 3 files changed, 81 insertions(+), 71 deletions(-) >=20 > @@ -40,16 +40,16 @@ static int xen_host_pci_sysfs_path(const XenHostPCI= Device *d, > d->domain, d->bus, d->dev, d->func, name); > =20 > if (rc >=3D size || rc < 0) { > - /* The output is truncated, or some other error was encountere= d */ > - return -ENODEV; > + /* The output is truncated, or some other error was encountere= d. > + * 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 >=3D 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] =3D 0; > rc =3D qemu_strtoul(buf, &endptr, base, &value); Do you still need a local 'value' variable, or can you just reuse pvalue here? > if (!rc) { > *pvalue =3D value; > + } else if (rc =3D=3D -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 =3D 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); > } > =20 > -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; > =20 > -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); > =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, > + &err); > + if (err) { > + error_append_hint(&err, "Failed to \"open\" the real pci devic= e"); 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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --DshQ1KnX8QuHOm3tShgncRfcAEKpwtxrA 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/ iQEcBAEBCAAGBQJWkD01AAoJEKeha0olJ0Nqd3MH/jsnnb3ZvYEbZbiDdci2i6MV 3LpLLy5s6hIyjMFRgFw8qJRw7rb8mfdq1oP6fNacu9I56CeO2abk4n2XApCcu/T5 MqPwL4RKURGyRyaZk7SEMeJi2LHKoSbgLKxym9HD+O/oo/Y+dcYGv413xE5AK36Q oHiBIXavpX6BoLxta0OYpeJjo/dLPYs4d2HfNUIaAIbKGwDyJBoHyFtPNrrxPY3N FN5fZ2zEs/zIw97+sOnrUL7Gg2ww4HQay2qV4tvWFMnc+7m0Xc1RJd0o4u/NIwXa 4AQ1Uhuv+Y+oeb++10u6wEVEZIvFE0ykLdNV92kaFSRSWv/4DZyEZwux3xsu0HI= =q/fA -----END PGP SIGNATURE----- --DshQ1KnX8QuHOm3tShgncRfcAEKpwtxrA--