From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id D8A12C433EF for ; Tue, 26 Apr 2022 07:40:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: In-Reply-To:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date: Reply-To:Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date :Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=rzBjDJ1UiVVWib9Q6u6Keeg9tsTi5Q//xrKwconRv7w=; b=FxWFIykbaTwUyVqGg2bFG/ob+I OAJEzeUhSjrdmnhMwbrYxUfFsMB7U3seuX4KCd5GbSfyd3qRitMjWd1w4jBSox4PqEkgspTXv4rOW leHH6tMCpKHgH5N5EGIGWUSdBWPeBy6arALID6asWraZEXt4e6YVtdjUd+Uyxvys0xt52BA4OyXTx segoR3sm4WMO011atr5KUIttjluKWvFlNZ6+wg9okqc2I46IsMup9LoPpKp2whgIDxQSbSfNNjlpj I0z1SXUziSv+1xxx3gLQsF0yGmYKb3w69dqi5HCrr8KWHuhUY3Lkttan9qrpB6ABsJZ0SPCSo060I Gvw8rrUA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1njFn0-00D1Zi-5L; Tue, 26 Apr 2022 07:39:14 +0000 Received: from relay4-d.mail.gandi.net ([217.70.183.196]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1njFmu-00D1WZ-2Z for linux-arm-kernel@lists.infradead.org; Tue, 26 Apr 2022 07:39:10 +0000 Received: (Authenticated sender: paul.kocialkowski@bootlin.com) by mail.gandi.net (Postfix) with ESMTPSA id A2192E000D; Tue, 26 Apr 2022 07:39:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1650958746; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=arsiKzPnEyF2K1/BzOvkvzG8lC0x6Q6kwRJkhLhmhqc=; b=pB+4G01PCbX6vzjTny2tsNFd2/EdAjTXpjMNgF30NWkaBIWJDVskdSErgd8gtY8QT+klgm bibLmZ9TFbs6AIZAWLI5+rDPsRPIu2nTCdaxwa2UB8JZIcn6gNGwTza4gOSFYIbNOYjn/1 WZDb/no7BWnlepJbynBFyQ20U6TQvpV4t92ZPIggULIbw75b6R5UbCEZgcuO1onm3KRdn8 78lzbNoBFN1U1hovxWSaOnH05KI9YiGdnocRj6B+4h/kaS+xZOKpeNWwJR6L8B6xdGs2hE ITTKj6Y5V/eU9aw7iWl4MG6Fg0ZPTz2nhbSEcB6sqQHoParsxZ9gqdQzBukCww== Date: Tue, 26 Apr 2022 09:39:03 +0200 From: Paul Kocialkowski To: Dan Carpenter Cc: Ian Cowan , mripard@kernel.org, mchehab@kernel.org, gregkh@linuxfoundation.org, wens@csie.org, jernej.skrabec@gmail.com, samuel@sholland.org, linux-media@vger.kernel.org, linux-staging@lists.linux.dev, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev Subject: Re: [PATCH] staging: sunxi: cedrus: centralize cedrus_open exit Message-ID: References: <20220423180111.91602-1-ian@linux.cowan.aero> <20220425092048.GL2462@kadam> <20220425100057.GB2090@kadam> MIME-Version: 1.0 In-Reply-To: <20220425100057.GB2090@kadam> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220426_003908_437630_CDF88792 X-CRM114-Status: GOOD ( 28.07 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: multipart/mixed; boundary="===============0301012608904522252==" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --===============0301012608904522252== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="eOxEJt1XwXWGH2/R" Content-Disposition: inline --eOxEJt1XwXWGH2/R Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Dan, On Mon 25 Apr 22, 13:00, Dan Carpenter wrote: > On Mon, Apr 25, 2022 at 11:29:30AM +0200, Paul Kocialkowski wrote: > > >=20 > > > No. You are just making the code ugly and complicated for no reason. > > >=20 > > > I work in static analysis so I have focussed a lot of attention on > > > locking bugs. In real life this theory is totally bogus. Single exit > > > paths only cause bugs, they don't prevent bugs. > >=20 > > I'm really surprised by this and honestly it feels a bit dogmatic. > >=20 > > It reminds me of CS teachers telling me "gotos are evil and you must > > never use them". In practice there are many situations where they make > > the code more readable and don't introduce any significant incertainty. >=20 > Gotos are fine. Backwards gotos are horrible, but sometimes necessary. > But pointless gotos are bad. And "out" is a bad label name. >=20 > return -ENOMEM; >=20 > That's perfectly readable. >=20 > ret =3D -ENOMEM; > goto out; >=20 > That's vague. The out label is likely to do nothing or everything. The > problem with do-nothing gotos are that people forget to set the error > code. Also it interrupts the reader, now you have to scroll to the > bottom of the function, you have lost your train of thought, and then > you have scroll back and find your place again. Okay I think these are good points, fair enough. > Do-everything gotos are the most bug prone style of error handling. > Imagine the function is trying to do three things. It fails part way > through. Now you're trying to undo the second thing which was never > done. Just moments ago I was just looking at one of these do-everything > bugs where it was using uninitialized memory. So by that you mean having just one label for all error handling instead of labels for each undo step? I've also seen conditionals used in error labels to undo stuff. Would you recommend duplicating error cleanup in each error condition? It feels like another set of issue on its own, besides the obvious downside of duplication. Thanks for the explanation! Paul > Another problem with do-everything error handling is that eventually it > gets too complicated so people just leave the error handling out. It's > hard to audit to see if everything is freed. >=20 > With static analysis, I'm mostly looking at error handling instead of > success paths paths. The one error label style is the by far the > worst. >=20 > People think single labels will prevent locking bugs. It doesn't work. > There is two people who use indenting to remind them about locks: >=20 > lock(); { > frob(); > frob(); > frob(); > } unlock(); >=20 > And occasionally they still forget to drop the lock before returning on > error paths. Nothing works for forgot to drop the lock bugs except > static analysis. >=20 > regards, > dan carpenter --=20 Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com --eOxEJt1XwXWGH2/R Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAEBCAAdFiEEJZpWjZeIetVBefti3cLmz3+fv9EFAmJnoZcACgkQ3cLmz3+f v9HEIgf/fqx7Nzmz0NvFjvbT/W0lmADI86JNWqQP/iDKn1QlEeLYz3mPwuyA37mL fq8FY7krBUUbrfpZRiN7H+6GHnnwmSY6Sm+UWmUqtUaF7fC7a5qDvHbVixCLTG36 g1f4pSb4+HUyEmsyDYpFilc0H+Vl2EzhfACwQlnntcF/CLl10TOUGDDx5tdSdtKn zuAGHHs+jF/8/xvp2UaYI5OlFc0NiZbbmXwzgcD4o78EkG2mBRGr0byLljRjs+uA 8ZcfLW5e9RNkhJWf0EFWcV0DdjBk0Uamb9cMpNn7d266/wnKCRbTGDrOhGfJIB+I Pb8BSp21xxHPNen+dwP6BcZEkRgc7Q== =hGdZ -----END PGP SIGNATURE----- --eOxEJt1XwXWGH2/R-- --===============0301012608904522252== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --===============0301012608904522252==--