From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933583AbcIUNGs (ORCPT ); Wed, 21 Sep 2016 09:06:48 -0400 Received: from mout.web.de ([212.227.17.12]:50606 "EHLO mout.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932959AbcIUNGq (ORCPT ); Wed, 21 Sep 2016 09:06:46 -0400 Subject: Re: virtio_console: Less function calls in init_vqs() after error detection To: Amit Shah References: <566ABCD9.1060404@users.sourceforge.net> <020438b9-a7f8-0050-04c1-43382ba60b75@users.sourceforge.net> <490b98e1-6129-f11f-55ff-94219ebce6d6@users.sourceforge.net> <20160921121038.GE1375@amit-lp.rh> Cc: virtualization@lists.linux-foundation.org, Arnd Bergmann , Greg Kroah-Hartman , "Michael S. Tsirkin" , Rusty Russell , LKML , kernel-janitors@vger.kernel.org, Julia Lawall From: SF Markus Elfring Message-ID: <4306f978-13dd-ca69-97f7-3424905a1d53@users.sourceforge.net> Date: Wed, 21 Sep 2016 15:06:25 +0200 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: <20160921121038.GE1375@amit-lp.rh> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Provags-ID: V03:K0:z4yL2/H4J2hUwQLGCR8/rvHPktXhcJ7XZywnO3QA/wHy94RvwgP FkM3KLGVI7lIWekwDyEmp15a2JPwyU+osX9/D6Rm36U6j28V85el333Hdkb77CtVMH6D2rJ gSo3dyeu4KPVPODsAGfdZ7Ly6TKf0M6rAsJbmRs6mCjirvGxgbAu0SZbJvjAyQViDk/egjq AMizIsyggZ8I4B0fk2QRA== X-UI-Out-Filterresults: notjunk:1;V01:K0:QNaF3qoQ5x4=:ptjAxsvvXXm0fWK9/dNVhD LNvn8TIKSHoYqH4+1Ug3m6B3QJh0LSrF8LHzKIWUn7z+zyNeazjudXd6Cusm1VHXI9J5q8cQ3 WUeaid7Oy2rdtpKrbxG4JGjxOx6GRIsJEsa2tDhJonqjQiKJ21Ro+XZ8aI4Sygn1efMt/Qzop S9Anst4faIDh424N2gObgKYOpQcIXXisfppYbcrT+PmYPrlikempxCHuXktAtE6K61xWAZca/ H3ihjHzLbSUhLztxSVpRlkukc/EpGJhgRQt2Y++OLUurDajNc4j53qmOFfJMk/BCszhzOtdIT PuYAYkveXLGeI1Zpp59Zwm2ZvHKbSG+X9nTTPf6wNrgvxj8MvitfqBodZhlWVKfOhOui79DCr 5yaXWYs5zDM7+3Uvyk612cLukYSOQ6tYMdIHFcGIWj4L48LiuOHgH65PteDH/nW+sYu7f3ha2 2WC5wQKqzUTIwakt5ZsYLalAnmaS+QvBPIO+yHjBlWWtAhx8MgR8Ik8w9d/HdOz92NObGV7F6 T6CJs1pDUy0RqZZDqFaToEkiNWfJ/oaWlXHXDhkk0ugVSdakYaGFwovQwwrdvZ/0NszRStJCT kThm8FtRb0xb2D+iIWuAESVYbxR39voU2lsqIJtgiOA819ZQY2VWZv1zjHSZBV5SsVPGPA1MB 4oaEGWH8okYpmasQBtlwiLanKRhjTCS6EFaedUylGz66//QPOGxdu08rwZ34xoWJnAdcYKX2O aHRueoZAxDyctD0peL+kvTV9aQ4Yzg3CdSleSm82K42OkowWEyR2LACaOzifeYqilxaKsTkr/ drehON1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org >> The kfree() function was called in up to five cases >> by the init_vqs() function during error handling even if >> the passed variable contained a null pointer. >> >> * Return directly after a call of the function "kmalloc_array" failed >> at the beginning. >> >> * Split a condition check for memory allocation 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 >> >> * Adjust jump targets according to the Linux coding style convention. > > So I've seen this series and I'm not yet sure how I feel about the > patches - f.e. in this one, it adds more lines than it removes to > achieve the same effect. I find this consequence still debatable. > I think the code is currently more readable than after these changes. Thanks for your constructive feedback. Can it be that an other software development concern is eventually overlooked? > And even if kfree is called multiple times, it isn't a huge bother I know also that the implementation of this function tolerates the passing of null pointers. > -- it's error case anyway, very unlikely to trigger, but keeps everything very readble. I suggest to reconsider this design detail if it is really acceptable for the safe implementation of such a software module. * How much will it matter in general that four function call were performed in this use case without checking their return values immediately? * Should it usually be determined quicker if a required resource like memory could be acquired before trying the next allocation? Regards, Markus