From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753952AbbBJVEy (ORCPT ); Tue, 10 Feb 2015 16:04:54 -0500 Received: from mail-ig0-f176.google.com ([209.85.213.176]:51873 "EHLO mail-ig0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752845AbbBJVEx (ORCPT ); Tue, 10 Feb 2015 16:04:53 -0500 Message-ID: <54DA7273.40100@kernel.dk> Date: Tue, 10 Feb 2015 14:04:51 -0700 From: Jens Axboe User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Nicholas Krause CC: linux-kernel@vger.kernel.org Subject: Re: [PATCH] block:Add proper error checking to genhd.c for the function,add_disk References: <1423531337-27297-1-git-send-email-xerofoify@gmail.com> In-Reply-To: <1423531337-27297-1-git-send-email-xerofoify@gmail.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 02/09/2015 06:22 PM, Nicholas Krause wrote: > Adds error handling to the function add_disk. Error checking is > added by removing the calls to WARN_ON when there is a failure in > various places in the function to proper return statements with > a error code. Further more due to this we must also add a statement > to return 0 at the end of the function to signal success to the > caller of the function to notify it that the function,add_disk has > succeeded and change the function's return type to a int now to > allow returning of error codes for the function,add_disk to be > allowed. There's a lot wrong with this still: > @@ -595,10 +594,9 @@ void add_disk(struct gendisk *disk) > disk->flags |= GENHD_FL_UP; > > retval = blk_alloc_devt(&disk->part0, &devt); > - if (retval) { > - WARN_ON(1); > - return; > - } > + if (retval) > + return -ENOMEM; Why isn't that returning retval? > + > disk_to_dev(disk)->devt = devt; > > /* ->major and ->first_minor aren't supposed to be > @@ -622,13 +620,16 @@ void add_disk(struct gendisk *disk) > * Take an extra ref on queue which will be put on disk_release() > * so that it sticks around as long as @disk is there. > */ > - WARN_ON_ONCE(!blk_get_queue(disk->queue)); > + if (!blk_get_queue(disk->queue)) > + return -ENODEV; You just leaked the devt alloc. > retval = sysfs_create_link(&disk_to_dev(disk)->kobj, &bdi->dev->kobj, > "bdi"); > - WARN_ON(retval); > - > + if (retval) > + return -EFAULT; Leaked devt alloc and queue ref. On top of that, some of the functions that add_disk() calls can fail, yet it doesn't check for those. And the most major part of this is ensuring that all callders of add_disk() properly check and handle the error it will pass back, that is completely ignored. Just making add_disk() error and unroll itself is the smaller part of the task. -- Jens Axboe