From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54122) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHDiz-0003GR-Vj for qemu-devel@nongnu.org; Thu, 07 Jan 2016 11:47:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aHDix-0007om-91 for qemu-devel@nongnu.org; Thu, 07 Jan 2016 11:47:45 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39466) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHDix-0007oi-0e for qemu-devel@nongnu.org; Thu, 07 Jan 2016 11:47:43 -0500 References: <1452047951-17429-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1452047951-17429-2-git-send-email-caoj.fnst@cn.fujitsu.com> <568D37F0.6080101@redhat.com> <568DD7D7.6010308@cn.fujitsu.com> From: Eric Blake Message-ID: <568E96A9.7040801@redhat.com> Date: Thu, 7 Jan 2016 09:47:37 -0700 MIME-Version: 1.0 In-Reply-To: <568DD7D7.6010308@cn.fujitsu.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="i3vwnorGO7sQ0C4ESq2DifKmW2Ridphda" 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) --i3vwnorGO7sQ0C4ESq2DifKmW2Ridphda Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 01/06/2016 08:13 PM, Cao jin wrote: >=20 >=20 > On 01/06/2016 11:51 PM, Eric Blake wrote: >> On 01/05/2016 07:39 PM, Cao jin wrote: >>> To catch the error msg. Also modify the caller >>> >>> Signed-off-by: Cao jin >>> --- >>> xen_host_pci_get_resource(XenHostPCIDevice *d) >>> >>> rc =3D xen_host_pci_sysfs_path(d, "resource", path, sizeof (pat= h)); >>> 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. >> >=20 > snprintf() is called in xen_host_pci_sysfs_path() above, and is the onl= y > possible error source, so I guess the errno is relevant? Then maybe it's better to pass Error **errp through xen_host_pci_sysfs_path(), so that the error message is closer to the real error. Especially since you have multiple callers, all identically affected (each caller shouldn't have to repeat the same common error-handling logic, if you can push it down lower in the callstack). For that matter, the only failure of xen_host_pci_sysfs_path is to return -ENODEV, but it does NOT set 'errno =3D ENODEV' on that error path= , so you most likely ARE printing the wrong errno value. Another option is to at least reword the message to make sense locally, as in: if (xen_host_pci_sysfs_path(...)) { error_setg(errp, "failed to determine device path for %s", name); return; } >=20 > Or, replace the error_setg_errno() to assert(0)? because if snprintf > goes wrong, user seems can do nothing. If you want to assert, then do that in xen_host_pci_sysfs_path(), not in all callers; at which point xen_host_pci_sysfs_path() should be fixed to return void. >>> +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_err'. >> >=20 > agree. I guess maybe "local_err" is a better mnemonic for newbie. and > when I am gradually familiar with error report, I also prefer "err". > Actually I considered to change this name, I just don`t want to bother > the reviewer to review it again, especially when the patch got a > Review-by and new version just changes some names. Will fix it. We don't mind re-reviewing a patch, but it does help if your cover letter and/or text after the --- divider explains how v4 differs from v3, to help us focus on the changes. >>> + 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 > Yes, but this will be fixed with error_propagate when patch "4/4 conver= t > to realize" comes, so is it ok to let it be here? No. Every patch should be stand-alone correct, or else strongly document why it is taking a shortcut that a later patch will clean up. At a bare minimum, if you don't fix the leak here, you should have a FIXME comment both here and in the commit message; but it's probably easier to just avoid the leak in the first place. >=20 > I originally want to make these patch independent from each other with > the most necessary modification. Splitting patches can be an art form. But your comment about the patch being independent _is_ the reason why the patch must not leak - if a backporter uses this patch but not the rest of the series, they should not be introducing a leak in their backport. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --i3vwnorGO7sQ0C4ESq2DifKmW2Ridphda 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/ iQEcBAEBCAAGBQJWjpatAAoJEKeha0olJ0Nqwx0H/0RABXI3I6iI9m3dwtKNZjLR sVTT+zJvFZlJH7yVluGUSugh3iDMnVJ5eYTu6azWU9OwcdC29rA7GIXg772iWcJa fG+uqFTOW4lhk4JqH/tm+yl+d6xYCsQCPanSCeVetqx3XapCFWXbLZgcvzbyOdLZ EHV4hVxqzOl9yaic+MpOZY4ULROvg31qQ4KPDtyNGmzSeBsRT2ZGjqDZSCpS8MB3 ng+tP/LTgtMFCflkFxDWova56Q9QqwIcsdDFVNc1hm15Yco8hwHsqCxUCIsTWNxM C/NKwk04LWn45lGL7jqSTN+POsGkzaaSUEp6ObImvBZ21Mo81MGkJnNpoZB3SRc= =VzUk -----END PGP SIGNATURE----- --i3vwnorGO7sQ0C4ESq2DifKmW2Ridphda--