Re: [dpdk-dev] [PATCH v7 05/12] eal: Fix uio mapping differences between linuxapp and bsdapp

[Bruce Richardson <bruce.richardson@intel.com>]

On Tue, Jun 30, 2015 at 05:24:21PM +0900, Tetsuya Mukawa wrote:
> From: "Tetsuya.Mukawa" <mukawa@igel.co.jp>
> 
> This patch fixes below.
> - bsdapp
>  - Use map_id in pci_uio_map_resource().
>  - Fix interface of pci_map_resource().
>  - Move path variable of mapped_pci_resource structure to pci_map.
> - linuxapp
>  - Remove redundant error message of linuxapp.
> 
> 'pci_uio_map_resource()' is implemented in both linuxapp and bsdapp,
> but interface is different. The patch fixes the function of bsdapp
> to do same as linuxapp. After applying it, file descriptor should be
> opened and closed out of pci_map_resource().
> 
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> ---
>  lib/librte_eal/bsdapp/eal/eal_pci.c       | 118 ++++++++++++++++++------------
>  lib/librte_eal/linuxapp/eal/eal_pci_uio.c |  21 +++---
>  2 files changed, 80 insertions(+), 59 deletions(-)
> 
<snip>  
>  free_uio_res:
> +	for (i = 0; i < map_idx; i++)
> +		rte_free(maps[i].path);
>  	rte_free(uio_res);
>  close_fd:
>  	close(dev->intr_handle.fd);

Comments on previous patch about merging error labels would also apply here.

> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> index c3b259b..19620fe 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> @@ -116,17 +116,11 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
>  					fd, (off_t)uio_res->maps[i].offset,
>  					(size_t)uio_res->maps[i].size, 0);
>  			if (mapaddr != uio_res->maps[i].addr) {
> -				if (mapaddr == MAP_FAILED)
> -					RTE_LOG(ERR, EAL,
> -							"Cannot mmap device resource file %s: %s\n",
> -							uio_res->maps[i].path,
> -							strerror(errno));
> -				else
> -					RTE_LOG(ERR, EAL,
> -							"Cannot mmap device resource file %s to address: %p\n",
> -							uio_res->maps[i].path,
> -							uio_res->maps[i].addr);
> -
> +				RTE_LOG(ERR, EAL,
> +						"Cannot mmap device resource "
> +						"file %s to address: %p\n",
> +						uio_res->maps[i].path,
> +						uio_res->maps[i].addr);
>  				close(fd);
>  				return -1;
>  			}
> @@ -353,8 +347,11 @@ pci_uio_map_resource(struct rte_pci_device *dev)
>  
>  		/* allocate memory to keep path */
>  		maps[map_idx].path = rte_malloc(NULL, strlen(devname) + 1, 0);
> -		if (maps[map_idx].path == NULL)
> +		if (maps[map_idx].path == NULL) {
> +			RTE_LOG(ERR, EAL, "Cannot allocate memory for "
> +					"path: %s\n", strerror(errno));

It's recommended not to split literal strings across multiple lines. This is
so that it's easy to find error messages in the source code. In this case, because
of the split, someone using git grep to search for the error message 
"Cannot allocate memory for path" 
would not be able to find it in the code. Longer lines are allowed in code for
literal strings.

/Bruce