Re: [Fuego] [PATCH] fio/parser.py: Add support to parse later versions of Fio 2.16

Thanks for this submission.  I have a few questions inline below.

> -----Original Message-----
> From: [email protected] <[email protected]=
m>
> Subject: RE: [PATCH] fio/parser.py: Add support to parse later versions o=
f Fio 2.16
>=20
>=20
> Dear Tim,
>=20
> I would like to add below change related to Fio parser to Upstream.
>=20
> Kindly review and provide your feedback.
>=20
>=20
> Thanks & Regards
> Sireesha
>=20
> -----Original Message-----
> From: [email protected] <[email protected]=
m>
> Sent: Wednesday, March 30, 2022 6:48 PM
> To: [email protected]
> Cc: nakkala sireesha(=1B$B#T#S#I#P=1B(B) <[email protected]=
om>; [email protected]; dinesh kumar(=1B$B#T#S#I#P=1B(B)
> <[email protected]>; hayashi kazuhiro(=1B$BNS=1B(B =1B$BOB9(=
=1B(B =1B$B""#S#W#C"~#A#C#T=1B(B) <[email protected]>; pyla v=
enkata(=1B$B#T#S#I=1B(B
> =1B$B#P=1B(B) <[email protected]>
> Subject: [PATCH] fio/parser.py: Add support to parse later versions of Fi=
o 2.16
>=20
> From: sireesha <[email protected]>
>=20
> The existing parser.py file works for fio version 2.0.8 which is the test=
 version defined and used in fuego tests.
>=20
> As Fio output has changed from 2.16 version, to use fio with fuego the pa=
rser is modified to parse the output of Fio version 2.0.8 and later
> version of 2.16
>=20
> Signed-off-by: sireesha <[email protected]>
> Signed-off-by: venkata pyla <[email protected]>
> ---
>  tests/Benchmark.fio/parser.py | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)  mode change 100755 =3D=
> 100644 tests/Benchmark.fio/parser.py
>=20
> diff --git a/tests/Benchmark.fio/parser.py b/tests/Benchmark.fio/parser.p=
y old mode 100755 new mode 100644 index 52a44e5..ef081b5
> --- a/tests/Benchmark.fio/parser.py
> +++ b/tests/Benchmark.fio/parser.py
> @@ -4,13 +4,30 @@
>  import os, sys
>  import common as plib
>=20
> -regex_string =3D '(  READ:|  WRITE:)(.*)(aggrb=3D)([\d.]+)([KM]B\/s)'
> +regex_string =3D ''
> +regex_string_old =3D '(  READ:|  WRITE:)(.*)(aggrb=3D)([\d.]+)([KM]B\/s)=
'
> +regex_string_new =3D '( READ:| WRITE:)(.*)(bw=3D.*)(.*)(\()([\d.]+)([kKM=
]B\/s)(\))'
regex_string_new has a rather perplexing sequence here:
(bw=3D.*)(.*)(\()
The opening paren can be used as a match to terminate that sequence, but
why are there two .* expressions back-to-back.  That's weird.

Also,
Both of these strings (the old and the new) use more groups than they reall=
y need to .
I know you are just copying the already-existing regex_string, but the only
groups actually used by the parser are (in the old) string group 3 and grou=
p 4.

It might be worth taking out the other unused groups, and just making these
strings:
regex_string_old =3D '(  READ:|  WRITE:).*aggrb=3D([\d.]+)([KM]B\/s)'
regex_string_new =3D '( READ:| WRITE:).*bw=3D.*\(([\d.]+)([kKM]B\/s)\)'

Then, you could just use groups 1 and 2 for both regex strings.
I think this simplifies the parser (and the patch), if it works.

I don't have the 2.16 version of fio on any of my boards.  Can you
test this there and let me know if it works.

> +test_version =3D ''
> +with open(plib.TEST_LOG,'r') as cur_file:
> +    raw_values =3D cur_file.readlines()
> +    test_version =3D raw_values[4].rstrip("\n")
> +
> +if test_version > '2.16':
Should this be >=3D instead of just '>'?

> +    regex_string =3D regex_string_new
> +else:
> +    regex_string =3D regex_string_old
> +
>  measurements =3D {}
>=20
>  # handle results in MB or KB
>  def set_measure(tguid, match):
> -    speed =3D match[3]
> -    units =3D match[4]
> +    if test_version > '2.16':
> +        speed =3D match[5]
> +        units =3D match[6]
> +    else:
> +        speed =3D match[3]
> +        units =3D match[4]

If you make the group numbers the same for both the old and new regex_strin=
gs,
you don't need this extra test.
With my above rewrite of the regext_strings, this would change to:
speed =3D match[1]
units =3D match[2]

and would work for both regex_string_old and regex_string_new.

>      multiplier =3D 1
>      if units.startswith('M'):
>          # WARNING - this depends on the value of kb_base specified to
> --
> 2.20.1
>=20

Can you please see if you can rewrite this parser with less groups in the r=
egex_strings,
and try to use the same group indices for the desired parse elements for bo=
th the
old and new regex strings?

Thanks,
 -- Tim