Re: [patch iproute2 v5 2/3] tc: Add -bs option to batch mode

[David Ahern <dsahern@gmail.com>]

You need a patch description here ...

On 1/2/18 7:55 PM, Chris Mi wrote:
>  static int tc_action_modify(int cmd, unsigned int flags,
> -			    int *argc_p, char ***argv_p)
> +			    int *argc_p, char ***argv_p,
> +			    int batch_size, int index, bool send)
>  {
>  	int argc = *argc_p;
>  	char **argv = *argv_p;
>  	int ret = 0;
> -	struct {
> -		struct nlmsghdr         n;
> -		struct tcamsg           t;
> -		char                    buf[MAX_MSG];
> -	} req = {
> -		.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcamsg)),
> -		.n.nlmsg_flags = NLM_F_REQUEST | flags,
> -		.n.nlmsg_type = cmd,
> -		.t.tca_family = AF_UNSPEC,
> +	tc_action_req *req;
> +	struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
> +	struct iovec *iov = &msg_iov[index];

Reverse xmas tree is the coding standard for net code. Please check all
new code to conform to this standard.

I have not reviewed all of this patch, but I firmly believe the batching
size option needs to be able handle a file with mixed commands. Your use
case is filter and action adds and deletes, but you should allow users
(e.g., test suites) to benefit from this performance speed up with test
cases that have single files with all of the commands.

For example,
$ cat tc.batch
qdisc add dev eth2 ingress
filter add dev eth2 ingress protocol ip pref 21 flower dst_ip
192.168.1.0/16 action drop
filter add dev eth2 ingress protocol ip pref 22 flower dst_ip
192.168.2.0/16 action drop
filter add dev eth2 ingress protocol ip pref 23 flower dst_ip
192.168.3.0/16 action drop
filter add dev eth2 ingress protocol ip pref 24 flower dst_ip
192.168.4.0/16 action drop
filter add dev eth2 ingress protocol ip pref 25 flower dst_ip
192.168.5.0/16 action drop
qdisc del dev eth2 ingress

(and consider this to be a huge file to really stress tc code paths for
example). Right now, the above file fails:

$ tc -b tc.batch -bs 5
Segmentation fault


Also, your changes fail to break out on an error:

$ tc -b tc.batch -bs 1
RTNETLINK answers: File exists
We have an error talking to the kernel, -1
RTNETLINK answers: File exists
We have an error talking to the kernel, -1
RTNETLINK answers: File exists
We have an error talking to the kernel, -1
RTNETLINK answers: File exists
We have an error talking to the kernel, -1
RTNETLINK answers: File exists
We have an error talking to the kernel, -1

where as the existing command does this:
$ tc -b tc.batch
RTNETLINK answers: File exists
We have an error talking to the kernel
Command failed tc.batch:1