netdev - Re: [PATCH iproute2-next 1/9] rdam: Add option to provide "-" sign for the port number

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 2 Jan 2018 19:46:38 -0700
From:   David Ahern <dsahern@...il.com>
To:     Leon Romanovsky <leon@...nel.org>,
        Doug Ledford <dledford@...hat.com>,
        Jason Gunthorpe <jgg@...lanox.com>
Cc:     RDMA mailing list <linux-rdma@...r.kernel.org>,
        Leon Romanovsky <leonro@...lanox.com>,
        netdev <netdev@...r.kernel.org>,
        Stephen Hemminger <stephen@...workplumber.org>
Subject: Re: [PATCH iproute2-next 1/9] rdam: Add option to provide "-" sign
 for the port number

a couple of nits

On 1/2/18 2:37 AM, Leon Romanovsky wrote:
> diff --git a/rdma/utils.c b/rdma/utils.c
> index 7b2001e2..7c920a5c 100644
> --- a/rdma/utils.c
> +++ b/rdma/utils.c
> @@ -10,6 +10,7 @@
>   */
>  
>  #include "rdma.h"
> +#include <ctype.h>
>  
>  static int rd_argc(struct rd *rd)
>  {
> @@ -50,13 +51,43 @@ bool rd_no_arg(struct rd *rd)
>  	return rd_argc(rd) == 0;
>  }
>  
> -uint32_t get_port_from_argv(struct rd *rd)
> +/*
> + * Possible input:output
> + * dev/port    | first port | is_dump_all
> + * mlx5_1      | 0          | true
> + * mlx5_1/     | 0          | true
> + * mlx5_1/0    | 0          | false
> + * mlx5_1/1    | 1          | false
> + * mlx5_1/-    | 0          | false
> + *
> + * In strict mode, /- will return error.
> + */
> +static int get_port_from_argv(struct rd *rd, uint32_t *port,
> +			      bool *is_dump_all, bool strict_port)
>  {
>  	char *slash;
>  
> +	*port = 0;
> +	*is_dump_all = true;
> +
>  	slash = strchr(rd_argv(rd), '/');
>  	/* if no port found, return 0 */
> -	return slash ? atoi(slash + 1) : 0;
> +	if (slash) {

++slash here would alleviate the need for all of the +1's.


> +		if (*(slash + 1) == '-') {
> +			if (strict_port)
> +				return -EINVAL;
> +			*is_dump_all = false;
> +			return 0;
> +		}
> +
> +		if (isdigit(*(slash + 1))) {
> +			*is_dump_all = false;
> +			*port = atoi(slash + 1);
> +		}
> +		if (!*port && strlen(slash + 1))
> +			return -EINVAL;
> +	}
> +	return 0;
>  }
>  
>  static struct dev_map *dev_map_alloc(const char *dev_name)
> @@ -152,7 +183,7 @@ void rd_free(struct rd *rd)
>  	dev_map_cleanup(rd);
>  }
>  
> -int rd_exec_link(struct rd *rd, int (*cb)(struct rd *rd))
> +int rd_exec_link(struct rd *rd, int (*cb)(struct rd *rd), bool strict_port)
>  {
>  	struct dev_map *dev_map;
>  	uint32_t port;
> @@ -163,7 +194,8 @@ int rd_exec_link(struct rd *rd, int (*cb)(struct rd *rd))
>  	if (rd_no_arg(rd)) {
>  		list_for_each_entry(dev_map, &rd->dev_map_list, list) {
>  			rd->dev_idx = dev_map->idx;
> -			for (port = 1; port < dev_map->num_ports + 1; port++) {
> +			port = (strict_port) ? 1 : 0;
> +			for (; port < dev_map->num_ports + 1; port++) {
>  				rd->port_idx = port;
>  				ret = cb(rd);
>  				if (ret)
> @@ -172,21 +204,22 @@ int rd_exec_link(struct rd *rd, int (*cb)(struct rd *rd))
>  		}
>  
>  	} else {
> +		bool is_dump_all;
>  		dev_map = dev_map_lookup(rd, true);

newline between declarations and code.

> -		port = get_port_from_argv(rd);
> -		if (!dev_map || port > dev_map->num_ports) {
> +		ret = get_port_from_argv(rd, &port, &is_dump_all, strict_port);
> +		if (!dev_map || port > dev_map->num_ports || (!port && ret)) {
>  			pr_err("Wrong device name\n");
>  			ret = -ENOENT;
>  			goto out;
>  		}
>  		rd_arg_inc(rd);
>  		rd->dev_idx = dev_map->idx;
> -		rd->port_idx = port ? : 1;
> +		rd->port_idx = port;
>  		for (; rd->port_idx < dev_map->num_ports + 1; rd->port_idx++) {
>  			ret = cb(rd);
>  			if (ret)
>  				goto out;
> -			if (port)
> +			if (!is_dump_all)
>  				/*
>  				 * We got request to show link for devname
>  				 * with port index.
>