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

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Mon, 7 Oct 2019 20:37:31 +0900
"Daniel T. Lee" <danieltimlee@gmail.com> wrote:

> 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?

I agree.  I'm fine with this approach. I've already ACK'ed the patch.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer