On Wed, Aug 18, 2021 at 1:50 AM Philipp Rudo <prudo(a)redhat.com> wrote:
On Tue, 17 Aug 2021 17:40:08 +0800
Kairui Song <kasong(a)redhat.com> wrote:
> On Thu, Aug 12, 2021 at 11:13 PM Philipp Rudo <prudo(a)redhat.com> wrote:
> >
> > Hi Kairui,
> >
> > On Thu, 12 Aug 2021 13:47:29 +0800
> > Kairui Song <kasong(a)redhat.com> wrote:
> >
> > > Avoid duplicated echo / cut call, this make it easier to clean up code
> > > in later commits.
> > >
> > > Signed-off-by: Kairui Song <kasong(a)redhat.com>
> > > ---
> > > dracut-module-setup.sh | 9 +++++++--
> > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/dracut-module-setup.sh b/dracut-module-setup.sh
> > > index 53abdc75..768026d3 100755
> > > --- a/dracut-module-setup.sh
> > > +++ b/dracut-module-setup.sh
> > > @@ -523,9 +523,14 @@ kdump_get_ip_route()
> > >
> > > kdump_get_ip_route_field()
> > > {
> > > - if `echo $1 | grep -q $2`; then
> > > - echo ${1##*$2} | cut -d ' ' -f1
> > > + local _str=$1
> > > +
> > > + if [[ "$_str" == *" $2 "* ]]; then
> > > + _str="${_str##*$2 }"
> > > + _str="${_str%% *}"
> > > fi
> > > +
> > > + echo "$_str"
> >
> > I see multiple problems with this function
> >
> > 1. both the old and new version have a problem with partial matches,
> > e.g. consider
> >
> > $1="something match what_i_want partial_match not_this"
> > ^
> > this space is important,
> > otherwise the new version
> > does not work
> > $2="match"
> >
> > both, the old and new version would return 'not_this'. On one hand
> > the newer version is a little better as it requires a full match to
> > enter the if-block on the other it now fails if the search term is
> > at the beginning of $1 and thus not proceeded by a space...
> >
> > 2. the new version returns the full $1 if no match was found. I would
> > expect it to return an empty string instead.
> >
> > I don't think the problems above cause any bug in today's uses
> > nevertheless I think the function should be hardened. The easiest fix
> > is probably to use sed again, i.e.
> > echo $1 | sed -n -e "s/\<$2\>\s\+\(\S\+\)\s.*$/\1/p"
> > should work
> >
> > Thanks
> > Philipp
> >
>
> Thanks for the review!
>
> How about:
> kdump_get_ip_route_field()
> {
> local _str=$1
>
> if [[ "$_str" == *" $2 "* ]]; then
> _str="${_str##* $2 }"
> # Note the space before $2 above
> _str="${_str%% *}"
> echo "$_str"
> fi
> }
>
> This should have the same effect as using `sed -n -e
> "s/.*\s\+\<$2\>\s\+\(\S\+\)\s.*$/\1/p"`
> and bash string substitution is much faster.
I'm not sure...
You are totally right that the bash string substitution is much faster
but that has the price that it is extremely rigid. For example the
new function you proposed still requires a _space_ before and after the
field name (which btw is not the same like the sed command as \s
matches any whitespace char, including tabs [1]) so it will still fail
when e.g. the field is at the beginning of the string or when a space
is replaced by a tab etc.. AFAIK there is no way to fix this with the
string substitution.
I believe in the end we need to decide what's more important. The
performance win or the additional flexibility (which hopefully results
in less bugs in the future).
The way I see it the function is only called twice and only for certain
setups. So I expect the over all performance win is neglectable when
looking at the over all dump time. That's why I vote for more
flexibility. But that's just my personal opinion.
Thanks
Philipp
[1]
https://www.gnu.org/software/sed/manual/html_node/regexp-extensions.html
Thanks for the suggestion.
I agree, maybe [[:space:]] can be used to match different spaces but
bash string substitution will make things ugly here indeed, just use
sed will make it much cleaner.
--
Best Regards,
Kairui Song