From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030317AbcIVUXI (ORCPT ); Thu, 22 Sep 2016 16:23:08 -0400 Received: from arroyo.ext.ti.com ([198.47.19.12]:41757 "EHLO arroyo.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934727AbcIVUXG (ORCPT ); Thu, 22 Sep 2016 16:23:06 -0400 Subject: Re: GPU-DRM-TILCDC: Less function calls in tilcdc_convert_slave_node() after error detection To: SF Markus Elfring References: <566ABCD9.1060404@users.sourceforge.net> <2f3f7ad7-16a0-1dfb-d073-0d993cd767ee@users.sourceforge.net> <0be7fee0-64f7-fa02-0337-51376677343e@users.sourceforge.net> CC: , David Airlie , Tomi Valkeinen , LKML , , Julia Lawall From: Jyri Sarha Message-ID: Date: Thu, 22 Sep 2016 23:22:48 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/22/16 21:38, SF Markus Elfring wrote: >>> The of_node_put() function was called in some cases >>> by the tilcdc_convert_slave_node() function during error handling >>> even if the passed variable contained a null pointer. >>> >>> * Adjust jump targets according to the Linux coding style convention. >>> >>> * Split a condition check for resource detection failures so that >>> each pointer from these function calls will be checked immediately. >>> >>> See also background information: >>> Topic "CWE-754: Improper check for unusual or exceptional conditions" >>> Link: https://cwe.mitre.org/data/definitions/754.html >>> >> >> I don't really agree with this patch. > > This kind of feedback can be fine at first glance. > > >> There is no harm in calling of_node_put() with NULL as an argument > > The cost of additional function calls will be eventually not noticed > just because they belong to an exception handling implementation so far. > > >> and because of that there is no point in making the function more complex > > There is inherent software complexity involved. > I think the "if (node)" in the of_node_put() is there on purpose, because it potentially saves the caller one extra if()-statement and keeps the caller code simpler. > >> and harder to maintain. > > How do you think about to discuss this aspect a bit more? > Keeping the goto labels in right order needs precision and can lead to subtle errors. Sometimes there is no way to avoid that, but here there is. Best regards, Jyri