From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dario Faggioli Subject: Re: [PATCH v2 02/10] libxl-pci: removing the extra 'out_closedir' cleaning path Date: Fri, 8 Apr 2016 10:12:56 +0200 Message-ID: <1460103176.13871.20.camel@citrix.com> References: <1459943163-18697-1-git-send-email-paulinaszubarczyk@gmail.com> <1459943163-18697-3-git-send-email-paulinaszubarczyk@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6213649198995371863==" Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1aoRXS-0003B7-UZ for xen-devel@lists.xenproject.org; Fri, 08 Apr 2016 08:13:11 +0000 In-Reply-To: <1459943163-18697-3-git-send-email-paulinaszubarczyk@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Paulina Szubarczyk , xen-devel@lists.xenproject.org, roger.pau@citrix.com, George.Dunlap@eu.citrix.com Cc: ian.jackson@eu.citrix.com, wei.liu2@citrix.com, ian.campbell@citrix.com List-Id: xen-devel@lists.xenproject.org --===============6213649198995371863== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-KQSjoOPG4pedtllUEW0O" --=-KQSjoOPG4pedtllUEW0O Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 2016-04-06 at 13:45 +0200, Paulina Szubarczyk wrote: > Signed-off-by: Paulina Szubarczyk >=20 > --- > Changed since v1: > =C2=A0* Modify the libxl_device_pci_assignable_list() function to use > =C2=A0=C2=A0=C2=A0only one 'out' cleaning path. > So, the patch does quite a few style cleanups. This is indeed a good thing, but both subject and changelog need to better reflect it. I'd go for the following. Subject: "libxl: style cleanups in=C2=A0libxl_device_pci_assignable_list() Changelog: "various coding style compliance cleanups, such as, arranging for=C2=A0 =C2=A0using only one path out of the function, whitespaces in loops ad if-s= =C2=A0 =C2=A0and r instead of rc for storing non-libxl error codes." Again, the code looks ok to me. Just one suggestion: > --- a/tools/libxl/libxl_pci.c > +++ b/tools/libxl/libxl_pci.c > @@ -395,34 +395,35 @@ libxl_device_pci > *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0libxl_device_pci *pcidevs =3D NULL, *new, *= assigned; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct dirent *de; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0DIR *dir; > -=C2=A0=C2=A0=C2=A0=C2=A0int rc, num_assigned; > +=C2=A0=C2=A0=C2=A0=C2=A0int r, num_assigned; > =C2=A0 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0*num =3D 0; > =C2=A0 > -=C2=A0=C2=A0=C2=A0=C2=A0rc =3D get_all_assigned_devices(gc, &assigned, &= num_assigned); > -=C2=A0=C2=A0=C2=A0=C2=A0if ( rc ) > +=C2=A0=C2=A0=C2=A0=C2=A0r =3D get_all_assigned_devices(gc, &assigned, &n= um_assigned); > +=C2=A0=C2=A0=C2=A0=C2=A0if (r) > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0goto out; > =C2=A0 libxl CODING_STYLE allows 'if (r) goto out;', which, since you're refactoring, you may want to use here. Regards, Dario --=20 <> (Raistlin Majere) ----------------------------------------------------------------- Dario Faggioli, Ph.D, http://about.me/dario.faggioli Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) --=-KQSjoOPG4pedtllUEW0O Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iEYEABECAAYFAlcHaAgACgkQk4XaBE3IOsTnDACeM5b6vIwswQxYlJMFgfJhblSI S9cAnjOIRGz/yH++luXLRmpwMvyMxQBS =Zisl -----END PGP SIGNATURE----- --=-KQSjoOPG4pedtllUEW0O-- --===============6213649198995371863== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============6213649198995371863==--