From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755509AbcIMMzH (ORCPT ); Tue, 13 Sep 2016 08:55:07 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:40870 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754858AbcIMMzF (ORCPT ); Tue, 13 Sep 2016 08:55:05 -0400 X-IBM-Helo: d06dlp03.portsmouth.uk.ibm.com X-IBM-MailFrom: borntraeger@de.ibm.com X-IBM-RcptTo: kernel-janitors@vger.kernel.org;linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/4] virtio_blk: Less function calls in init_vq() after error detection To: SF Markus Elfring , virtualization@lists.linux-foundation.org, "Michael S. Tsirkin" References: <566ABCD9.1060404@users.sourceforge.net> <02054675-8395-ac81-6863-e3a5cbfc9032@users.sourceforge.net> Cc: Julia Lawall , kernel-janitors@vger.kernel.org, LKML From: Christian Borntraeger Date: Tue, 13 Sep 2016 14:54:58 +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: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16091312-0024-0000-0000-00000217D145 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16091312-0025-0000-0000-0000205BD275 Message-Id: X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-09-13_08:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=2 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1609020000 definitions=main-1609130190 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/13/2016 02:13 PM, SF Markus Elfring wrote: > From: Markus Elfring > Date: Tue, 13 Sep 2016 13:20:44 +0200 > > The kfree() function was called in up to three cases > by the init_vq() function during error handling even if > the passed variable contained a null pointer. > > * Split a condition check for memory allocation failures. > > * Adjust jump targets according to the Linux coding style convention. > > Signed-off-by: Markus Elfring > --- > drivers/block/virtio_blk.c | 22 +++++++++++++++++----- > 1 file changed, 17 insertions(+), 5 deletions(-) Can't you see from this diffstat that the patch actually seems to makes the code more complex? In addition, please have a look at commit 347a529398e8e723338cca5d8a8ae2d9e7e93448 virtio_blk: Fix a slient kernel panic which did the opposite of your patch. And in fact it fixed a bug. Quite obviously multiple labels are harder to read and harder to get right. For error handling with just kfree one label is just the right thing to. > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > index 6553eb7..d28dbcf 100644 > --- a/drivers/block/virtio_blk.c > +++ b/drivers/block/virtio_blk.c > @@ -395,11 +395,21 @@ static int init_vq(struct virtio_blk *vblk) > return -ENOMEM; > > names = kmalloc_array(num_vqs, sizeof(*names), GFP_KERNEL); > + if (!names) { > + err = -ENOMEM; > + goto free_vblk_vqs; > + } > + > callbacks = kmalloc_array(num_vqs, sizeof(*callbacks), GFP_KERNEL); > + if (!callbacks) { > + err = -ENOMEM; > + goto free_names; > + } > + > vqs = kmalloc_array(num_vqs, sizeof(*vqs), GFP_KERNEL); > - if (!names || !callbacks || !vqs) { > + if (!vqs) { > err = -ENOMEM; > - goto out; > + goto free_callbacks; > } > > for (i = 0; i < num_vqs; i++) { > @@ -411,19 +421,21 @@ static int init_vq(struct virtio_blk *vblk) > /* Discover virtqueues and write information to configuration. */ > err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names); > if (err) > - goto out; > + goto free_vqs; > > for (i = 0; i < num_vqs; i++) { > spin_lock_init(&vblk->vqs[i].lock); > vblk->vqs[i].vq = vqs[i]; > } > vblk->num_vqs = num_vqs; > - > -out: > + free_vqs: > kfree(vqs); > + free_callbacks: > kfree(callbacks); > + free_names: > kfree(names); > if (err) > + free_vblk_vqs: > kfree(vblk->vqs); > return err; > } >