Re: [PATCH v2 05/49] kdumpctl: use kdump_get_conf_val to read config values

[Kairui Song]

On Fri, Sep 3, 2021 at 8:15 PM Philipp Rudo <prudo(a)redhat.com&gt; wrote:
>  Hi Kairui,
>
>  On Thu, 19 Aug 2021 19:39:04 +0800
>  Kairui Song <kasong(a)redhat.com&gt; wrote:
>
>  > Signed-off-by: Kairui Song <kasong(a)redhat.com&gt;
>  > ---
>  >  kdumpctl | 47 +++++++++++++++++++++--------------------------
>  >  1 file changed, 21 insertions(+), 26 deletions(-)
>  >
>  > diff --git a/kdumpctl b/kdumpctl
>  > index db3a3015..50e92cb4 100755
>  > --- a/kdumpctl
>  > +++ b/kdumpctl
>  > @@ -339,8 +339,8 @@ check_files_modified()
>  >       #also rebuild when Pacemaker cluster conf is changed and fence kdump is
> enabled.
>  >       modified_files=$(get_pcs_cluster_modified_files)
>  >
>  > -     EXTRA_BINS=`grep ^kdump_post $KDUMP_CONFIG_FILE | cut -d\  -f2`
>  > -     CHECK_FILES=`grep ^kdump_pre $KDUMP_CONFIG_FILE | cut -d\  -f2`
>  > +     EXTRA_BINS=$(kdump_get_conf_val kdump_post)
>  > +     CHECK_FILES=$(kdump_get_conf_val kdump_pre)
>  >       HOOKS="/etc/kdump/post.d/ /etc/kdump/pre.d/"
>  >       if [ -d /etc/kdump/post.d ]; then
>  >               for file in /etc/kdump/post.d/*; do
>  > @@ -357,17 +357,17 @@ check_files_modified()
>  >               done
>  >       fi
>  >       HOOKS="$HOOKS $POST_FILES $PRE_FILES"
>  > -     CORE_COLLECTOR=`grep ^core_collector $KDUMP_CONFIG_FILE | cut -d\  -f2`
>  > +     CORE_COLLECTOR=$(kdump_get_conf_val core_collector | cut -d ' ' -f 1)
>
>  keeping this cut here still makes you vulnerable to some of the problems
>  you solved by introducing kdump_get_conf_val, e.g. when the core_collector is
>  followed by a tab instead of a space. As this is a bash only file you could
>  make CORE_COLLECTOR an array and then use the first element as command. 
I think simply use a awk '{print $1}' should be enough just for this case.
>  BTW in general this function could do with a do over, e.g. files and
>  modified_fiels could be made an array as well. Furthermore some variables seem
>  unnecessary to me and the others probably should be made local.
>  Anyway as the series is already pretty long I think both can wait for the next
>  cleanup series. 
Great suggestion, actually I found a lot of extra problems during
making this series and recorded some on my local to-do list. Let's fix
it later, currently, let me just update this series to avoid
introducing any new issue.

--
Best Regards,
Kairui Song