[v4,0/7] net/route: rewrite route-change-{dst, gw, if} into new API
mbox series

Message ID 20190903141610.28887-1-pvorel@suse.cz
Headers show
Series
  • net/route: rewrite route-change-{dst, gw, if} into new API
Related show

Message

Petr Vorel Sept. 3, 2019, 2:16 p.m. UTC
Hi Alexey,

this version address functionality problems you pointed out.
But yet there might need to be another version as I'm not sure about
tst_ipaddr_un() API changes.

Changes v3-v4:
* enhanced tst_ipaddr_un() with -b, -h -l, -m, -n and -p options (-p was
previous -m)
* use tst_ipaddr_un() options to fix address clash on host_id, net_id clash fix
by adjusting $1 in test (this is really inconsistent, see note at 3/7).
* added tests for tst_ipaddr_un()
* quiet EXPECT_PASS ping$TST_IPV6

Kind regards,
Petr

Petr Vorel (7):
  tst_net.sh: enhance tst_add_ipaddr(), add tst_del_ipaddr()
  tst_net.sh: Add -p option to return prefix in tst_ipaddr_un()
  tst_net.sh: Add (mostly) HOST_ID related options to tst_ipaddr_un
  net: Add tst_ipaddr_un.sh test
  network/route: Rewrite route{4,6}-change-dst into new shell API
  network/route: Rewrite route{4,6}-change-gw into new shell API
  network/route: Rewrite route{4,6}-change-if into new API

 lib/newlib_tests/net/tst_ipaddr_un.sh         | 239 +++++++++++++
 runtest/net_stress.route                      |  15 +-
 testcases/lib/tst_net.sh                      | 196 +++++++++--
 .../network/stress/route/00_Descriptions.txt  |  54 +--
 .../network/stress/route/route-change-dst.sh  |  34 ++
 .../network/stress/route/route-change-gw.sh   |  39 +++
 .../network/stress/route/route-change-if.sh   |  90 +++++
 testcases/network/stress/route/route-lib.sh   |  17 +
 .../network/stress/route/route4-change-dst    | 276 ---------------
 .../network/stress/route/route4-change-gw     | 292 ----------------
 .../network/stress/route/route4-change-if     | 324 ------------------
 .../network/stress/route/route6-change-dst    | 272 ---------------
 .../network/stress/route/route6-change-gw     | 292 ----------------
 .../network/stress/route/route6-change-if     | 323 -----------------
 14 files changed, 595 insertions(+), 1868 deletions(-)
 create mode 100755 lib/newlib_tests/net/tst_ipaddr_un.sh
 create mode 100755 testcases/network/stress/route/route-change-dst.sh
 create mode 100755 testcases/network/stress/route/route-change-gw.sh
 create mode 100755 testcases/network/stress/route/route-change-if.sh
 create mode 100644 testcases/network/stress/route/route-lib.sh
 delete mode 100644 testcases/network/stress/route/route4-change-dst
 delete mode 100644 testcases/network/stress/route/route4-change-gw
 delete mode 100644 testcases/network/stress/route/route4-change-if
 delete mode 100644 testcases/network/stress/route/route6-change-dst
 delete mode 100644 testcases/network/stress/route/route6-change-gw
 delete mode 100644 testcases/network/stress/route/route6-change-if

Comments

Alexey Kodanev Sept. 12, 2019, 12:56 p.m. UTC | #1
On 9/3/19 5:16 PM, Petr Vorel wrote:
> Hi Alexey,
> 
> this version address functionality problems you pointed out.
> But yet there might need to be another version as I'm not sure about
> tst_ipaddr_un() API changes.
> 
> Changes v3-v4:
> * enhanced tst_ipaddr_un() with -b, -h -l, -m, -n and -p options (-p was
> previous -m)
> * use tst_ipaddr_un() options to fix address clash on host_id, net_id clash fix
> by adjusting $1 in test (this is really inconsistent, see note at 3/7).
> * added tests for tst_ipaddr_un()
> * quiet EXPECT_PASS ping$TST_IPV6
> 

The patch-set looks good, the only concerns are:

* complicated tst_ipaddr_un(), may be add another high-level function
  to use raw tst_ipadd_un(), if -b, -f, -n options are really needed?

* if NS_TIMES is large, there are a lot of messages in a test output
  (it seems NS_TIMES * 4), could we minimize the number of them?
Petr Vorel Sept. 12, 2019, 1:48 p.m. UTC | #2
Hi Alexey,

first, thanks for your review.

..
> > this version address functionality problems you pointed out.
> > But yet there might need to be another version as I'm not sure about
> > tst_ipaddr_un() API changes.

> > Changes v3-v4:
> > * enhanced tst_ipaddr_un() with -b, -h -l, -m, -n and -p options (-p was
> > previous -m)
> > * use tst_ipaddr_un() options to fix address clash on host_id, net_id clash fix
> > by adjusting $1 in test (this is really inconsistent, see note at 3/7).
> > * added tests for tst_ipaddr_un()
> > * quiet EXPECT_PASS ping$TST_IPV6


> The patch-set looks good, the only concerns are:

> * complicated tst_ipaddr_un(), may be add another high-level function
>   to use raw tst_ipadd_un(), if -b, -f, -n options are really needed?
Sure. I'll keep only -l and -m and drop -b, -f, -n and -h as not needed ATM.
And add them via high-level function only if needed.

In that case I'd like to have -l and -m functionality also for NET_ID (already
needed in route-change-dst.sh). But not sure which options would be for it
(it'd be easy with long opts, but we don't want to depend on /usr/bin/getopt,
nor to parse it manually although of course doable)

Another option is to have single option for adding both MAX and MIN:

-h MIN,MAX # for HOST_ID
-n MIN,MAX # for NET_ID

(e.g. -n5,255 -h1,255)

But it looks to me a bit uncomfortable having to always to add both min and max.

> * if NS_TIMES is large, there are a lot of messages in a test output
>   (it seems NS_TIMES * 4), could we minimize the number of them?

I can add -q option to tst_add_ipaddr() which usage will make it NS_TIMES * 2. (1)
If it's still too much I'll drop "testing route over ..." (2)
Other option would be to print only even Nth iteration (I don't think it's a
good idea) or even has only single TPASS/TFAIL.

BTW don't we want end testing on first failure?


Kind regards,
Petr
Alexey Kodanev Sept. 12, 2019, 2:18 p.m. UTC | #3
On 9/12/19 4:48 PM, Petr Vorel wrote:
> Hi Alexey,
> 
> first, thanks for your review.
> 
> ..
>>> this version address functionality problems you pointed out.
>>> But yet there might need to be another version as I'm not sure about
>>> tst_ipaddr_un() API changes.
> 
>>> Changes v3-v4:
>>> * enhanced tst_ipaddr_un() with -b, -h -l, -m, -n and -p options (-p was
>>> previous -m)
>>> * use tst_ipaddr_un() options to fix address clash on host_id, net_id clash fix
>>> by adjusting $1 in test (this is really inconsistent, see note at 3/7).
>>> * added tests for tst_ipaddr_un()
>>> * quiet EXPECT_PASS ping$TST_IPV6
> 
> 
>> The patch-set looks good, the only concerns are:
> 
>> * complicated tst_ipaddr_un(), may be add another high-level function
>>   to use raw tst_ipadd_un(), if -b, -f, -n options are really needed?
> Sure. I'll keep only -l and -m and drop -b, -f, -n and -h as not needed ATM.
> And add them via high-level function only if needed.
> 
> In that case I'd like to have -l and -m functionality also for NET_ID (already
> needed in route-change-dst.sh). But not sure which options would be for it
> (it'd be easy with long opts, but we don't want to depend on /usr/bin/getopt,
> nor to parse it manually although of course doable)
> 
> Another option is to have single option for adding both MAX and MIN:
> 
> -h MIN,MAX # for HOST_ID
> -n MIN,MAX # for NET_ID
> 
> (e.g. -n5,255 -h1,255)
>

It can be the best option,

 
> But it looks to me a bit uncomfortable having to always to add both min and max.

max "-n ,254" or min "-n 2"?


> 
>> * if NS_TIMES is large, there are a lot of messages in a test output
>>   (it seems NS_TIMES * 4), could we minimize the number of them?
> 
> I can add -q option to tst_add_ipaddr() which usage will make it NS_TIMES * 2. (1)
> If it's still too much I'll drop "testing route over ..." (2)
> Other option would be to print only even Nth iteration (I don't think it's a
> good idea) or even has only single TPASS/TFAIL.
> 
> BTW don't we want end testing on first failure?

Agree, why not.


> 
> 
> Kind regards,
> Petr
>
Petr Vorel Sept. 12, 2019, 3:07 p.m. UTC | #4
Hi Alexey,

> > Another option is to have single option for adding both MAX and MIN:

> > -h MIN,MAX # for HOST_ID
> > -n MIN,MAX # for NET_ID

> > (e.g. -n5,255 -h1,255)

> It can be the best option,


> > But it looks to me a bit uncomfortable having to always to add both min and max.

> max "-n ,254" or min "-n 2"?
Great, thanks! How simple and elegant solution :).


> >> * if NS_TIMES is large, there are a lot of messages in a test output
> >>   (it seems NS_TIMES * 4), could we minimize the number of them?

> > I can add -q option to tst_add_ipaddr() which usage will make it NS_TIMES * 2. (1)
> > If it's still too much I'll drop "testing route over ..." (2)
> > Other option would be to print only even Nth iteration (I don't think it's a
> > good idea) or even has only single TPASS/TFAIL.

> > BTW don't we want end testing on first failure?

> Agree, why not.
OK, it'll be in v5.

I'd prefer to change EXPECT_{PASS,FAIL} to return exit value so we can detect
the failure, but that would expect tst_res to return exit code as well.
(not sure, if it's a radical change).

Kind regards,
Petr