Hi Kairui,
On Mon, 13 Sep 2021 04:26:35 +0800
Kairui Song <kasong(a)redhat.com> wrote:
On Fri, Sep 3, 2021 at 8:15 PM Philipp Rudo <prudo(a)redhat.com>
wrote:
>
> Hi Kairui,
>
> On Thu, 19 Aug 2021 19:39:04 +0800
> Kairui Song <kasong(a)redhat.com> wrote:
>
> > Signed-off-by: Kairui Song <kasong(a)redhat.com>
> > ---
> > 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.
yes, using awk should do the trick here. But a quick search gave a few
other locations with similar usage of cut. So not sure if it's better to
fix it now or wait until you fix the other locations.
>
> 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.
fully agree. I just wanted to write it down so I don't forget it myself.
Thanks
Philipp