| Message ID | 20190914151353.18054-2-danieltimlee@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v3,1/3] samples: pktgen: make variable consistent with option | expand |
On Sun, 15 Sep 2019 00:13:52 +0900 "Daniel T. Lee" <danieltimlee@gmail.com> wrote: > This commit adds CIDR parsing and IP validate helper function to parse > single IP or range of IP with CIDR. (e.g. 198.18.0.0/15) > > Helpers will be used in prior to set target address in samples/pktgen. > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> > --- > Changes since v3: > * Set errexit option to stop script execution on error > > samples/pktgen/functions.sh | 124 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 124 insertions(+) > > diff --git a/samples/pktgen/functions.sh b/samples/pktgen/functions.sh > index 4af4046d71be..87ae61701904 100644 > --- a/samples/pktgen/functions.sh > +++ b/samples/pktgen/functions.sh > @@ -5,6 +5,8 @@ > # Author: Jesper Dangaaard Brouer > # License: GPL > > +set -o errexit Unfortunately, this breaks the scripts. The function proc_cmd are designed to grep after "Result: OK:" which might fail, and your patch/change makes the script stop immediately. We actually want to continue, and output what command that failed (and also grep again after "Result:" to provide the kernel reason). Even if you somehow "fix" function proc_cmd, then we in general want to catch different error situations by looking at status $?, and output meaning full errors via calling err() function. IHMO as minimum with errexit you need a 'trap' function that can help/inform the user of what went wrong.
Jesper Dangaard Brouer <brouer@redhat.com> writes: > On Sun, 15 Sep 2019 00:13:52 +0900 > "Daniel T. Lee" <danieltimlee@gmail.com> wrote: > >> This commit adds CIDR parsing and IP validate helper function to parse >> single IP or range of IP with CIDR. (e.g. 198.18.0.0/15) >> >> Helpers will be used in prior to set target address in samples/pktgen. >> >> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> >> --- >> Changes since v3: >> * Set errexit option to stop script execution on error >> >> samples/pktgen/functions.sh | 124 ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 124 insertions(+) >> >> diff --git a/samples/pktgen/functions.sh b/samples/pktgen/functions.sh >> index 4af4046d71be..87ae61701904 100644 >> --- a/samples/pktgen/functions.sh >> +++ b/samples/pktgen/functions.sh >> @@ -5,6 +5,8 @@ >> # Author: Jesper Dangaaard Brouer >> # License: GPL >> >> +set -o errexit > > Unfortunately, this breaks the scripts. > > The function proc_cmd are designed to grep after "Result: OK:" which > might fail, and your patch/change makes the script stop immediately. > We actually want to continue, and output what command that failed (and > also grep again after "Result:" to provide the kernel reason). > > Even if you somehow "fix" function proc_cmd, then we in general want to > catch different error situations by looking at status $?, and output > meaning full errors via calling err() function. Yeah, I can see that some functions do this, but I don't think that would be too hard to fix. See sample diff below (will need some more work to deal with grep failing, but you get the idea). I'd argue that fixing this is the right thing to do... Maybe add set -o nounset as well while we're at it? :) > IHMO as minimum with errexit you need a 'trap' function that can > help/inform the user of what went wrong. Yeah, trap ERR (which would also need set -o errtrace to work inside the functions) might be useful in any case. -Toke diff --git a/samples/pktgen/functions.sh b/samples/pktgen/functions.sh index 4af4046d71be..d61a348f85f5 100644 --- a/samples/pktgen/functions.sh +++ b/samples/pktgen/functions.sh @@ -58,6 +58,7 @@ function pg_set() { function proc_cmd() { local result local proc_file=$1 + local status=0 # after shift, the remaining args are contained in $@ shift local proc_ctrl=${PROC_DIR}/$proc_file @@ -73,8 +74,7 @@ function proc_cmd() { echo "cmd: $@ > $proc_ctrl" fi # Quoting of "$@" is important for space expansion - echo "$@" > "$proc_ctrl" - local status=$? + echo "$@" > "$proc_ctrl" || status=$? result=$(grep "Result: OK:" $proc_ctrl) # Due to pgctrl, cannot use exit code $? from grep
Thanks for the feedback!
How about just capturing with "Result: OK" except for 'pgctrl'?
(more details are in the diff below)
~/git/linux/net$ ag -Q 'Result:'
core/pktgen.c
702: seq_printf(seq, "Result: %s\n", pkt_dev->result);
704: seq_puts(seq, "Result: Idle\n");
1739: seq_printf(seq, "\nResult: %s\n", t->result);
1741: seq_puts(seq, "\nResult: NA\n");
The upper command shows that 'Result: ' string is only printed on
'pktgen_if' and 'pktgen_thread'. So I thought just changing to below diff
will solve the problem.
diff --git a/samples/pktgen/functions.sh b/samples/pktgen/functions.sh
index 87ae61701904..38cf9f62502f 100644
--- a/samples/pktgen/functions.sh
+++ b/samples/pktgen/functions.sh
@@ -60,6 +60,7 @@ function pg_set() {
function proc_cmd() {
local result
local proc_file=$1
+ local status=0
# after shift, the remaining args are contained in $@
shift
local proc_ctrl=${PROC_DIR}/$proc_file
@@ -75,13 +76,14 @@ function proc_cmd() {
echo "cmd: $@ > $proc_ctrl"
fi
# Quoting of "$@" is important for space expansion
- echo "$@" > "$proc_ctrl"
- local status=$?
+ echo "$@" > "$proc_ctrl" || status=$?
- result=$(grep "Result: OK:" $proc_ctrl)
- # Due to pgctrl, cannot use exit code $? from grep
- if [[ "$result" == "" ]]; then
- grep "Result:" $proc_ctrl >&2
+ if [[ "$proc_file" != "pgctrl" ]]; then
+ result=$(grep "Result: OK:" $proc_ctrl) || true
+ # Due to pgctrl, cannot use exit code $? from grep
+ if [[ "$result" == "" ]]; then
+ grep "Result:" $proc_ctrl >&2
+ fi
fi
> We actually want to continue, and output what command that failed (and
> also grep again after "Result:" to provide the kernel reason).
Since, with 'true', the command won't fail and continue, and on error with
'pktgen_if' and 'pktgen_thread', It'll grep again after "Result:" to provide
the kernel reason. [1][2]
> I'd argue that fixing this is the right thing to do... Maybe add set -o
> nounset as well while we're at it? :)
Adding nounset option could break working with $IP6. Most of the scripts
tries to check if $IP6 variable exists whether it is defined or not.
> Even if you somehow "fix" function proc_cmd, then we in general want to
> catch different error situations by looking at status $?, and output
> meaning full errors via calling err() function. IHMO as minimum with
> errexit you need a 'trap' function that can help/inform the user of
> what went wrong.
I agree. trap ERR will help with more sophisticated handling of errors, but
I'm not sure which to elaborate more (about what went wrong) compared
to current error format? (which is grep again after "Result:" to provide the
kernel reason.)
I really appreciate your time and effort for the review.
Thanks,
Daniel
[1]: pktgen_if_show:
https://elixir.bootlin.com/linux/latest/source/net/core/pktgen.c#L702
[2]: pktgen_thread_show:
https://elixir.bootlin.com/linux/latest/source/net/core/pktgen.c#L1739
diff --git a/samples/pktgen/functions.sh b/samples/pktgen/functions.sh index 4af4046d71be..87ae61701904 100644 --- a/samples/pktgen/functions.sh +++ b/samples/pktgen/functions.sh @@ -5,6 +5,8 @@ # Author: Jesper Dangaaard Brouer # License: GPL +set -o errexit + ## -- General shell logging cmds -- function err() { local exitcode=$1 @@ -163,6 +165,128 @@ function get_node_cpus() echo $node_cpu_list } +# Extend shrunken IPv6 address. +# fe80::42:bcff:fe84:e10a => fe80:0:0:0:42:bcff:fe84:e10a +function extend_addr6() +{ + local addr=$1 + local sep=: sep2=:: + local sep_cnt=$(tr -cd $sep <<< $1 | wc -c) + local shrink + + # separator count : should be between 2, 7. + if [[ $sep_cnt -lt 2 || $sep_cnt -gt 7 ]]; then + err 5 "Invalid IP6 address sep: $1" + fi + + # if shrink '::' occurs multiple, it's malformed. + shrink=( $(egrep -o "$sep{2,}" <<< $addr) ) + if [[ ${#shrink[@]} -ne 0 ]]; then + if [[ ${#shrink[@]} -gt 1 || ( ${shrink[0]} != $sep2 ) ]]; then + err 5 "Invalid IP$IP6 address shr: $1" + fi + fi + + # add 0 at begin & end, and extend addr by adding :0 + [[ ${addr:0:1} == $sep ]] && addr=0${addr} + [[ ${addr: -1} == $sep ]] && addr=${addr}0 + echo "${addr/$sep2/$(printf ':0%.s' $(seq $[8-sep_cnt])):}" +} + + +# Given a single IP(v4/v6) address, whether it is valid. +function validate_addr() +{ + # check function is called with (funcname)6 + [[ ${FUNCNAME[1]: -1} == 6 ]] && local IP6=6 + local len=$[ IP6 ? 8 : 4 ] + local max=$[ 2**(len*2)-1 ] + local addr sep + + # set separator for each IP(v4/v6) + [[ $IP6 ]] && sep=: || sep=. + IFS=$sep read -a addr <<< $1 + + # array length + if [[ ${#addr[@]} != $len ]]; then + err 5 "Invalid IP$IP6 address: $1" + fi + + # check each digit between 0, $max + for digit in "${addr[@]}"; do + [[ $IP6 ]] && digit=$[ 16#$digit ] + if [[ $digit -lt 0 || $digit -gt $max ]]; then + err 5 "Invalid IP$IP6 address: $1" + fi + done + + return 0 +} + +function validate_addr6() { validate_addr $@ ; } + +# Given a single IP(v4/v6) or CIDR, return minimum and maximum IP addr. +function parse_addr() +{ + # check function is called with (funcname)6 + [[ ${FUNCNAME[1]: -1} == 6 ]] && local IP6=6 + local bitlen=$[ IP6 ? 128 : 32 ] + local octet=$[ IP6 ? 16 : 8 ] + + local addr=$1 + local net prefix + local min_ip max_ip + + IFS='/' read net prefix <<< $addr + [[ $IP6 ]] && net=$(extend_addr6 $net) + validate_addr$IP6 $net + + if [[ $prefix -gt $bitlen ]]; then + err 5 "Invalid prefix: $prefix" + elif [[ -z $prefix ]]; then + min_ip=$net + max_ip=$net + else + # defining array for converting Decimal 2 Binary + # 00000000 00000001 00000010 00000011 00000100 ... + local d2b='{0..1}{0..1}{0..1}{0..1}{0..1}{0..1}{0..1}{0..1}' + [[ $IP6 ]] && d2b+=$d2b + eval local D2B=($d2b) + + local remain=$[ bitlen-prefix ] + local min_mask max_mask + local min max + local ip_bit + local ip sep + + # set separator for each IP(v4/v6) + [[ $IP6 ]] && sep=: || sep=. + IFS=$sep read -ra ip <<< $net + + min_mask="$(printf '1%.s' $(seq $prefix))$(printf '0%.s' $(seq $remain))" + max_mask="$(printf '0%.s' $(seq $prefix))$(printf '1%.s' $(seq $remain))" + + # calculate min/max ip with &,| operator + for i in "${!ip[@]}"; do + digit=$[ IP6 ? 16#${ip[$i]} : ${ip[$i]} ] + ip_bit=${D2B[$digit]} + + idx=$[ octet*i ] + min[$i]=$[ 2#$ip_bit & 2#${min_mask:$idx:$octet} ] + max[$i]=$[ 2#$ip_bit | 2#${max_mask:$idx:$octet} ] + [[ $IP6 ]] && { min[$i]=$(printf '%X' ${min[$i]}); + max[$i]=$(printf '%X' ${max[$i]}); } + done + + min_ip=$(IFS=$sep; echo "${min[*]}") + max_ip=$(IFS=$sep; echo "${max[*]}") + fi + + echo $min_ip $max_ip +} + +function parse_addr6() { parse_addr $@ ; } + # Given a single or range of port(s), return minimum and maximum port number. function parse_ports() {
This commit adds CIDR parsing and IP validate helper function to parse single IP or range of IP with CIDR. (e.g. 198.18.0.0/15) Helpers will be used in prior to set target address in samples/pktgen. Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com> --- Changes since v3: * Set errexit option to stop script execution on error samples/pktgen/functions.sh | 124 ++++++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+)