From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-f173.google.com (mail-wi0-f173.google.com [209.85.212.173]) by dpdk.org (Postfix) with ESMTP id 657D558C4 for ; Thu, 2 Oct 2014 09:40:44 +0200 (CEST) Received: by mail-wi0-f173.google.com with SMTP id bs8so3017914wib.6 for ; Thu, 02 Oct 2014 00:47:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:organization :user-agent:in-reply-to:references:mime-version :content-transfer-encoding:content-type; bh=bQJwyvBBNQ6HqtSmCbMX+AaENFAEc7lAQD+u21wZ6bc=; b=KInNmJXxsYgkQPdmgcpXebwSANSqsUVEontTTdRXRSGkhETh5LtX1NJrXSLyYv3PjF +o0hufEMQRYvR7FkTX4En8uzmuz5q7YIa8LsDcHU4teM9zw61PgwJukSPRnj4HvLQt5p n5briymboLzEARchWZUbHe/UMjat4s1stUasoKMzbMlL/KcomLm47zJht45/Oj99uLJw Y/pVTxWZuCy+R8beYcdg/uO/SzG9m4OOPQ8f1C7gW3H7JceGql66StN32ILYBVD8LZeQ bdiH0s2FLm02JIFDk7kaG7Y+r+xs5mNszPpGs3+uS4xSI5XaQZ1eNF+s1yb0hxzBGnIV Eurg== X-Gm-Message-State: ALoCoQkt+pBnvNwR4aGmvFkUuD+f+/hTwBogZlNCLckZRW1SHUq/YztY6QwzTk4WIaw1aNtuCu4k X-Received: by 10.194.103.41 with SMTP id ft9mr70971805wjb.93.1412236052604; Thu, 02 Oct 2014 00:47:32 -0700 (PDT) Received: from xps13.localnet (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177]) by mx.google.com with ESMTPSA id hp2sm2946577wjb.40.2014.10.02.00.47.31 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 02 Oct 2014 00:47:31 -0700 (PDT) From: Thomas Monjalon To: Bruce Richardson , Neil Horman Date: Thu, 02 Oct 2014 09:47:19 +0200 Message-ID: <2861812.mN4X60CA8c@xps13> Organization: 6WIND User-Agent: KMail/4.13.3 (Linux/3.15.8-1-ARCH; KDE/4.13.3; x86_64; ; ) In-Reply-To: <20141001160548.GA6676@BRICHA3-MOBL> References: <20141001160109.GE24028@localhost.localdomain> <20141001160548.GA6676@BRICHA3-MOBL> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v2] rte_mempool_dump() crashes with NULL rte_mempool pointer. X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Oct 2014 07:40:44 -0000 2014-10-01 17:05, Bruce Richardson: > On Wed, Oct 01, 2014 at 12:01:10PM -0400, Neil Horman wrote: > > On Wed, Oct 01, 2014 at 04:43:10PM +0100, Bruce Richardson wrote: > > > On Wed, Oct 01, 2014 at 11:02:27AM -0400, Neil Horman wrote: > > > > On Wed, Oct 01, 2014 at 03:36:45PM +0200, Thomas Monjalon wrote: > > > > > 2014-09-28 08:27, Neil Horman: > > > > > > On Sun, Sep 28, 2014 at 05:28:44AM +0000, Wiles, Roger Keith wrote: > > > > > > > Check the FILE *f and rte_mempool *mp pointers for NULL and > > > > > > > return plus print out a message if RTE_LIBRTE_MEMPOOL_DEBUG is enabled. > > > > > > > > > > > > > > Signed-off-by: Keith Wiles > > > > > > > > > > > > I'm fine with this, as I think passing in a NULL mempool is clearly a bug here, > > > > > > thats worth panicing over, though I wouldnt mind if we did a RTE_VERIFY_WARN > > > > > > macro here instead using what I suggested in my other note > > > > > > > > > > Passing a NULL mempool to rte_mempool_dump() is a bug in the application. > > > > > If you look elsewhere in the DPDK code, you'll see that it's not common to do > > > > > such check on input parameters. > > > > > A similar discussion already happened few months ago: > > > > > http://dpdk.org/ml/archives/dev/2014-June/003900.html > > > > > > > > > Not sure what your point is here Thomas. I think we're all in agreement that > > > > NULL is a bad value to pass in here. Are you asserting that we shouldn't bother > > > > with a NULL check at all and just accept the crash as it is? > > > > > > > > > > In the general case: > > > * Code in the datapath should not have things like NULL checks > > > * However, datapath code should generally have a debug option which turns > > > these checks on to help debugging if needed. > > > * Code not in the datapath probably should have these checks. > > > > > Ok, I can understand that, but I would hope that rte_mempool_dump isn't in the > > datapath, its rather by definition a debug function, isn't it? > > Neil > > Yes, agreed. [So it probably should have the NULL check]. I have many arguments to not do this check: 1) If it was a coding rule to do this kind of check, it should be done in almost every functions. 2) It's quite common to not do this check, e.g. what happen with memcpy(NULL,NULL)? 3) Why check only NULL value? 1 and 2 are also some invalid values... -- Thomas