From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Monjalon <thomas.monjalon@6wind.com>
Subject: Re: [PATCH] eal: fix check number of bytes from read
	function
Date: Thu, 21 Jul 2016 16:35:44 +0200
Message-ID: <1862165.0hl8WtbyGd@xps13>
References: <1469024689-1076-1-git-send-email-michalx.k.jastrzebski@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Cc: dev@dpdk.org, bruce.richardson@intel.com, michalx.kobylinski@intel.com,
 sergio.gonzalez.monroy@intel.com, david.marchand@6wind.com
To: Michal Jastrzebski <michalx.k.jastrzebski@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mail-wm0-f41.google.com (mail-wm0-f41.google.com [74.125.82.41])
 by dpdk.org (Postfix) with ESMTP id 4582D5586
 for <dev@dpdk.org>; Thu, 21 Jul 2016 16:35:46 +0200 (CEST)
Received: by mail-wm0-f41.google.com with SMTP id q128so24122324wma.1
 for <dev@dpdk.org>; Thu, 21 Jul 2016 07:35:46 -0700 (PDT)
In-Reply-To: <1469024689-1076-1-git-send-email-michalx.k.jastrzebski@intel.com>
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

Hi,

2016-07-20 16:24, Michal Jastrzebski:
> -	if (read(fd, &page, sizeof(uint64_t)) < 0) {
> +
> +	retval = read(fd, &page, sizeof(uint64_t));
> +	if (retval < 0) {
>  		RTE_LOG(ERR, EAL, "%s(): cannot read /proc/self/pagemap: %s\n",
>  				__func__, strerror(errno));
>  		close(fd);
>  		return RTE_BAD_PHYS_ADDR;
> +	}	else if (retval >= 0 && retval < (int)sizeof(uint64_t))	{

I have 4 comments about the above line:
- the check retval >= 0 is not needed because implied by else
- why not checking retval != sizeof(uint64_t) as it is the exact expected value?
- (int)sizeof(uint64_t) can be replaced by 8 but it's shorter ;)
- only 1 space is required between } and else

> +		RTE_LOG(ERR, EAL, "%s(): read %d bytes from /proc/self/pagemap "
> +				"but expected %d: %s\n",
> +				__func__, retval, (int)sizeof(uint64_t), strerror(errno));

Are you sure errno is meaningful here?

> +		close(fd);
> +		return RTE_BAD_PHYS_ADDR;
>  	}