On Mon, 7 Oct 2019 20:37:31 +0900
"Daniel T. Lee" <danieltim...@gmail.com> wrote:
> On Mon, Oct 7, 2019 at 5:15 PM Jesper Dangaard Brouer <bro...@redhat.com>
> wrote:
> >
> > On Sat, 5 Oct 2019 17:25:07 +0900
> > "Daniel T. Lee" <danieltim...@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 <danieltim...@gmail.com>
> > > ---
> >
> > Acked-by: Jesper Dangaard Brouer <bro...@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
