| Message ID | 20190903141610.28887-1-pvorel@suse.cz |
|---|---|
| Headers | show |
| Series | net/route: rewrite route-change-{dst, gw, if} into new API | expand |
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?
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
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 >
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
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