Re: [v3 2/3] samples: pktgen: add helper functions for IP(v4/v6) CIDR parsing

["Toke Høiland-Jørgensen" <toke@redhat.com>]

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