From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <tiwai@suse.de>
Received: from ozlabs.org (ozlabs.org [203.10.76.45])
	(using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))
	(Client CN "mx.ozlabs.org",
	Issuer "CA Cert Signing Authority" (verified OK))
	by bilbo.ozlabs.org (Postfix) with ESMTPS id 025C5B7BEF
	for <linuxppc-dev@lists.ozlabs.org>;
	Fri, 31 Jul 2009 16:23:00 +1000 (EST)
Received: from mx1.suse.de (cantor.suse.de [195.135.220.2])
	(using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))
	(Client CN "mx1.suse.de", Issuer "CAcert Class 3 Root" (not verified))
	by ozlabs.org (Postfix) with ESMTPS id 90B0ADDDA2
	for <linuxppc-dev@ozlabs.org>; Fri, 31 Jul 2009 16:22:59 +1000 (EST)
Date: Fri, 31 Jul 2009 08:22:49 +0200
Message-ID: <s5hws5pid6u.wl%tiwai@suse.de>
From: Takashi Iwai <tiwai@suse.de>
To: Julia Lawall <julia@diku.dk>
Subject: Re: [PATCH 5/5] sound/aoa: Add kmalloc NULL tests
In-Reply-To: <Pine.LNX.4.64.0907301627260.8734@ask.diku.dk>
References: <Pine.LNX.4.64.0907301610550.8734@ask.diku.dk>
	<1248963420.16129.0.camel@johannes.local>
	<Pine.LNX.4.64.0907301627260.8734@ask.diku.dk>
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Cc: linuxppc-dev@ozlabs.org, Johannes Berg <johannes@sipsolutions.net>,
	kernel-janitors@vger.kernel.org, alsa-devel@alsa-project.org,
	linux-kernel@vger.kernel.org
List-Id: Linux on PowerPC Developers Mail List <linuxppc-dev.lists.ozlabs.org>
List-Unsubscribe: <https://lists.ozlabs.org/options/linuxppc-dev>,
	<mailto:linuxppc-dev-request@lists.ozlabs.org?subject=unsubscribe>
List-Archive: <http://lists.ozlabs.org/pipermail/linuxppc-dev>
List-Post: <mailto:linuxppc-dev@lists.ozlabs.org>
List-Help: <mailto:linuxppc-dev-request@lists.ozlabs.org?subject=help>
List-Subscribe: <https://lists.ozlabs.org/listinfo/linuxppc-dev>,
	<mailto:linuxppc-dev-request@lists.ozlabs.org?subject=subscribe>

At Thu, 30 Jul 2009 16:29:54 +0200 (CEST),
Julia Lawall wrote:
> 
> On Thu, 30 Jul 2009, Johannes Berg wrote:
> 
> > On Thu, 2009-07-30 at 16:11 +0200, Julia Lawall wrote:
> > > From: Julia Lawall <julia@diku.dk>
> > > 
> > > Check that the result of kzalloc is not NULL before a dereference.
> > 
> > >  		irq_client = kzalloc(sizeof(struct pmf_irq_client),
> > >  				     GFP_KERNEL);
> > > +		if (!irq_client) {
> > > +			err = -ENOMEM;
> > > +			printk(KERN_ERR "snd-aoa: gpio layer failed to"
> > > +				" register %s irq (%d)\n", name, err);
> > > +			goto out_unlock;
> > > +		}
> > 
> > Looks good, thanks, but I'd really drop the printk if only to not have
> > the string there, that doesn't really seem interesting.
> 
> The printk is based on similar error handling code a few lines later:

But another problem is that the same error message is reused although
the error condition is totally different.  The kzalloc NULL isn't
about the registration error.  So, it's rather confusing.

However, for this particular error path, I agree with Johannes; we can
skip the error message since the error code ENOMEM is obvious.


thanks,

Takashi

> 
>                if (err) {
>                         printk(KERN_ERR "snd-aoa: gpio layer failed to"
>                                         " register %s irq (%d)\n", name, 
> err);
>                         kfree(irq_client);
>                         goto out_unlock;
>                 }
> 
> Should the printk be removed in this case as well?  Or is it ok to fail 
> silently in one case and not in the other?
> 
> julia
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>