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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAEKGpzintX9g5aBsN=qRuV12d3fPMicx+2Y1bKbOzUbw-X_ksg@mail.gmail.com>
Date:   Fri, 20 Sep 2019 12:04:15 +0900
From:   "Daniel T. Lee" <danieltimlee@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Jesper Dangaard Brouer <brouer@...hat.com>,
        "David S . Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [v3 2/3] samples: pktgen: add helper functions for IP(v4/v6) CIDR parsing

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