Re: [PATCH net-next v5 2/4] samples: pktgen: fix proc_cmd command result check logic

["Daniel T. Lee" <danieltimlee@gmail.com>]

On Mon, Oct 7, 2019 at 5:15 PM Jesper Dangaard Brouer <brouer@redhat.com> wrote:
>
> On Sat,  5 Oct 2019 17:25:07 +0900
> "Daniel T. Lee" <danieltimlee@gmail.com> wrote:
>
> > Currently, proc_cmd is used to dispatch command to 'pg_ctrl', 'pg_thread',
> > 'pg_set'. proc_cmd is designed to check command result with grep the
> > "Result:", but this might fail since this string is only shown in
> > 'pg_thread' and 'pg_set'.
> >
> > This commit fixes this logic by grep-ing the "Result:" string only when
> > the command is not for 'pg_ctrl'.
> >
> > For clarity of an execution flow, 'errexit' flag has been set.
> >
> > To cleanup pktgen on exit, trap has been added for EXIT signal.
> >
> > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > ---
>
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
>
> > Changes since v5:
> >  * when script runs sudo, run 'pg_ctrl "reset"' on EXIT with trap
> >
> >  samples/pktgen/functions.sh | 17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/samples/pktgen/functions.sh b/samples/pktgen/functions.sh
> > index 4af4046d71be..40873a5d1461 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
> > @@ -58,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
> > @@ -73,13 +76,13 @@ 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
> > +        if [[ "$result" == "" ]]; then
> > +            grep "Result:" $proc_ctrl >&2
> > +        fi
> >      fi
> >      if (( $status != 0 )); then
> >       err 5 "Write error($status) occurred cmd: \"$@ > $proc_ctrl\""
> > @@ -105,6 +108,8 @@ function pgset() {
> >      fi
> >  }
> >
> > +[[ $EUID -eq 0 ]] && trap 'pg_ctrl "reset"' EXIT
> > +
>
> This is fine, but you could have placed the 'trap' handler in
> parameters.sh file, as all scripts first source functions.sh and then
> call root_check_run_with_sudo, before sourcing parameters.sh.
>

Yes, this will work since 'parameters.sh' is only sourced when it is
on root, but I've thought this file only focuses on parsing parameters
not the general workflow of the pktgen script.

So I thought 'functions.sh' is more suitable place to add trap rather
than 'paramters.sh'.

What do you think?

Thanks for the review.

Thanks,
Daniel

> >  ## -- General shell tricks --
> >
> >  function root_check_run_with_sudo() {
>
>
>
> --
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   LinkedIn: http://www.linkedin.com/in/brouer