[PATCH 0/5] dts: testpmd show port info/stats

[PATCH 0/5] dts: testpmd show port info/stats

[Luca Vizzarro]

Hello,

As previously mentioned in the DTS meeting, here is the patch series
relating show port info and stats. It also includes a proposal for a
generic parsing utility.

From my existing testpmd params series I recognise there are quite a few
improvements that can also be added to this series. But I am reserving
them for v2. Feel free to point them out here as well.

Best,
Luca

Luca Vizzarro (5):
  dts: fix InteractiveShell command prompt filtering
  dts: skip first line of send_command output
  dts: add parsing utility module
  dts: add `show port info` command to TestPmdShell
  dts: add `show port stats` command to TestPmdShell

 dts/framework/parser.py                       | 147 +++++
 .../remote_session/interactive_shell.py       |  10 +-
 dts/framework/remote_session/testpmd_shell.py | 531 +++++++++++++++++-
 3 files changed, 685 insertions(+), 3 deletions(-)
 create mode 100644 dts/framework/parser.py

-- 
2.34.1

[PATCH 1/5] dts: fix InteractiveShell command prompt filtering

[Luca Vizzarro]

When sending a command using an instance of InteractiveShell the output
is meant to filter out the leading shell prompt. The filtering logic is
present but the line is appended anyways.

Bugzilla ID: 1411
Fixes: 88489c0501af ("dts: add smoke tests")

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
---
Cc: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/framework/remote_session/interactive_shell.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index 5cfe202e15..8a9bf96ea9 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -132,11 +132,11 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
         self._stdin.flush()
         out: str = ""
         for line in self._stdout:
-            out += line
             if prompt in line and not line.rstrip().endswith(
                 command.rstrip()
             ):  # ignore line that sent command
                 break
+            out += line
         self._logger.debug(f"Got output: {out}")
         return out
 
-- 
2.34.1

Re: [PATCH 1/5] dts: fix InteractiveShell command prompt filtering

[Juraj Linkeš]

On Fri, Apr 12, 2024 at 1:11 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> When sending a command using an instance of InteractiveShell the output
> is meant to filter out the leading shell prompt. The filtering logic is
> present but the line is appended anyways.
>

I don't think that's what's happening here. The output collecting
logic is "stop when we encounter a prompt, but not the prompt with the
command we sent". We could change the comment though.

> Bugzilla ID: 1411
> Fixes: 88489c0501af ("dts: add smoke tests")
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
> ---
> Cc: Jeremy Spewock <jspewock@iol.unh.edu>
> ---
>  dts/framework/remote_session/interactive_shell.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> index 5cfe202e15..8a9bf96ea9 100644
> --- a/dts/framework/remote_session/interactive_shell.py
> +++ b/dts/framework/remote_session/interactive_shell.py
> @@ -132,11 +132,11 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
>          self._stdin.flush()
>          out: str = ""
>          for line in self._stdout:
> -            out += line
>              if prompt in line and not line.rstrip().endswith(
>                  command.rstrip()
>              ):  # ignore line that sent command
>                  break
> +            out += line

If we do this, we'll only filter out the last prompt, which may not be
desirable, since the last prompt is there only because all of our
interactive shells force an extra prompt with _command_extra_chars.

One thing we could improve though is removing the distribution welcome
message from logs, or at least separate it from the first command sent
with the interactive shell. The second option will allow us to see
clearly that an interactive session has been established, although we
could just emit a shorter log (something like "Started a testpmd
session" and then flush the welcome screen output).

>          self._logger.debug(f"Got output: {out}")
>          return out
>
> --
> 2.34.1
>

Re: [PATCH 1/5] dts: fix InteractiveShell command prompt filtering

[Luca Vizzarro]

Thank you for your review Juraj!

On 16/04/2024 09:40, Juraj Linkeš wrote:

>> When sending a command using an instance of InteractiveShell the output
>> is meant to filter out the leading shell prompt. The filtering logic is
>> present but the line is appended anyways.
>>
> 
> I don't think that's what's happening here. The output collecting
> logic is "stop when we encounter a prompt, but not the prompt with the
> command we sent". We could change the comment though.

Yeah, I actually identified the behaviour better after writing this. 
Will update the commit body accordingly. And I mixed-up leading with 
trailing! This is meant to say "trailing shell prompt".

>> @@ -132,11 +132,11 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
>>           self._stdin.flush()
>>           out: str = ""
>>           for line in self._stdout:
>> -            out += line
>>               if prompt in line and not line.rstrip().endswith(
>>                   command.rstrip()
>>               ):  # ignore line that sent command
>>                   break
>> +            out += line
> 
> If we do this, we'll only filter out the last prompt, which may not be
> desirable, since the last prompt is there only because all of our
> interactive shells force an extra prompt with _command_extra_chars.

Could you please expand more on this?

> One thing we could improve though is removing the distribution welcome
> message from logs, or at least separate it from the first command sent
> with the interactive shell. The second option will allow us to see
> clearly that an interactive session has been established, although we
> could just emit a shorter log (something like "Started a testpmd
> session" and then flush the welcome screen output).

I am not sure what you are referring to exactly, could you also expand 
more on this please?

Given it's not particularly explained, I thought having two command 
prompts (especially a trailing one) was an error. The main reason behind 
this is that when we go to parse the port info, the last entry which is 
"device private info" appears to be open ended, and I couldn't gather 
much information from the testpmd source code. So I opted to parse 
everything until the end. With a trailing command prompt this meant 
that: device_private_info="....testpmd> ".

Re: [PATCH 1/5] dts: fix InteractiveShell command prompt filtering

[Juraj Linkeš]

On Tue, Apr 16, 2024 at 2:12 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> Thank you for your review Juraj!
>
> On 16/04/2024 09:40, Juraj Linkeš wrote:
>
> >> When sending a command using an instance of InteractiveShell the output
> >> is meant to filter out the leading shell prompt. The filtering logic is
> >> present but the line is appended anyways.
> >>
> >
> > I don't think that's what's happening here. The output collecting
> > logic is "stop when we encounter a prompt, but not the prompt with the
> > command we sent". We could change the comment though.
>
> Yeah, I actually identified the behaviour better after writing this.
> Will update the commit body accordingly. And I mixed-up leading with
> trailing! This is meant to say "trailing shell prompt".
>
> >> @@ -132,11 +132,11 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
> >>           self._stdin.flush()
> >>           out: str = ""
> >>           for line in self._stdout:
> >> -            out += line
> >>               if prompt in line and not line.rstrip().endswith(
> >>                   command.rstrip()
> >>               ):  # ignore line that sent command
> >>                   break
> >> +            out += line
> >
> > If we do this, we'll only filter out the last prompt, which may not be
> > desirable, since the last prompt is there only because all of our
> > interactive shells force an extra prompt with _command_extra_chars.
>
> Could you please expand more on this?
>

Actually, filtering out the last prompt is fine. I got this mixed up
with processing the last prompt. We must process the last prompt, but
we don't need to print it (and there's not much reason to do so).

And the reason we must process it is because we're consuming the
output line by line. A line appears in (or is yielded by) self._stdout
only after a newline appears in the output. For a typical command
output:
prompt> command\n
<command output>\n
prompt>

The last line in self._stdout is the last line of <command output>
because that's the last line with a newline. We send
_command_extra_chars to force another prompt into the output:
prompt> command\n
<command output>\n
prompt>\n
prompt>

Now the last line with a newline contains a prompt. The actual extra
trailing prompt is then pushed to the next command execution:
prompt_from_command1> command2\n
<command2 output>\n
<prompt_from_command2>

Maybe you already knew this but maybe we could archive this and point
to it in case anyone asks. :-)

> > One thing we could improve though is removing the distribution welcome
> > message from logs, or at least separate it from the first command sent
> > with the interactive shell. The second option will allow us to see
> > clearly that an interactive session has been established, although we
> > could just emit a shorter log (something like "Started a testpmd
> > session" and then flush the welcome screen output).
>
> I am not sure what you are referring to exactly, could you also expand
> more on this please?
>

Let's look at logs. The first thing to note is we don't log when we
start the interactive session. The first log from the session is the
output from the first command, such as testpmd:
2024/04/11 13:29:27 - test_suite - dts.SUT 1 - INFO - Sending: 'sudo
-- sh -c '/tmp/dpdk-22.07/x86_64-linux-native-gcc/app/dpdk-testpmd -l
1-2 -n 4 --file-prefix=dpdk_71695_20240411132902 -a 0000:07:00.0 -a
0000:08:00.0  -- -i --nb-cores=1 --port-topology=paired --numa
--tx-ip=198.18.0.1,198.18.0.2 --tx-udp=9 --forward-mode=io
--hairpin-mode=0x0 --rx-offloads=0x0 --rx-mq-mode=0x7
--tx-offloads=0x0 --mask-event=intr_lsc --no-mlockall''
2024/04/11 13:29:28 - test_suite - dts.SUT 1 - DEBUG - Got output:
Welcome to Ubuntu 22.04.2 LTS (GNU/Linux 5.15.0-97-generic x86_64)^M
<the welcome screen continues>
Last login: Thu Apr 11 11:24:44 2024 from 192.168.122.1^M^M
sudo -- sh -c '/tmp/dpdk-22.07/x86_64-linux-native-gcc/app/dpdk-testpmd
-l 1-2 -n 4 --file-prefix=dpdk_71695_20240411132902 -a 0000:07:00.0 -a
0000:08:00.0  -- -i --nb-cores=1 --port-topology=paired --numa
--tx-ip=198.18.0.1,198.18.0.2 --tx-udp=9 --forward-mode=io
--hairpin-mode=0x0 --rx-offloads=0x0 --rx-mq-mode=0x7
--tx-offloads=0x0 --mask-event=intr_lsc --no-mlockall'^M
^M
^[[?2004hjlinkes@dts-sut:~$ sudo -- sh -c
'/tmp/dpdk-22.07/x86_64-linux-native-gcc/app/dpdk-testpmd -l 1-2 -n 4
--file-prefix=dpdk_71695_20240411132902 -a 0000:07:00.0 -a
0000:08:00.0  -- -i --nb-cores=1 --port-topology=paired --numa
--tx-ip=198.18.0.1,198.18.0.2 --tx-udp=9 --forward-mode=io
--hairpin-mode=0x0 --rx-offloads=0x0 --rx-mq-mode=0x7
--tx-offloads=0x0 --mask-event=intr_lsc --no-mlockall'^M
^[[?2004l^M^M
EAL: Detected CPU lcores: 4^M
<tespmd startup continues>
Done^M
testpmd> ^M^M

2024/04/11 13:29:28 - test_suite - dts.SUT 1 - INFO - Sending: 'show
device info all'

What we see is the testpmd command being sent and then the output is:
Welcome screen
Testpmd command we've sent
Testpmd command echoed with the os prompt
Testpmd output
The next command

What's extra is the welcome screen with the first command we've sent.
We don't capture any output right after establishing the channel so it
all stays there to be captured by the first command we send.

> Given it's not particularly explained, I thought having two command
> prompts (especially a trailing one) was an error. The main reason behind
> this is that when we go to parse the port info, the last entry which is
> "device private info" appears to be open ended, and I couldn't gather
> much information from the testpmd source code. So I opted to parse
> everything until the end. With a trailing command prompt this meant
> that: device_private_info="....testpmd> ".

So the command output doesn't end with a newline? What's the exact
output that's captured?

Re: [PATCH 1/5] dts: fix InteractiveShell command prompt filtering

[Luca Vizzarro]

On 17/04/2024 14:06, Juraj Linkeš wrote:
> Actually, filtering out the last prompt is fine. I got this mixed up
> with processing the last prompt. We must process the last prompt, but
> we don't need to print it (and there's not much reason to do so).

Yeah, my confusion was about displaying the last prompt indeed. I 
recognise we need to process it to use it as an end reading symbol.

> And the reason we must process it is because we're consuming the
> output line by line. A line appears in (or is yielded by) self._stdout
> only after a newline appears in the output. For a typical command
> output:
> prompt> command\n
> <command output>\n
> prompt>
> 
> The last line in self._stdout is the last line of <command output>
> because that's the last line with a newline. We send
> _command_extra_chars to force another prompt into the output:
> prompt> command\n
> <command output>\n
> prompt>\n
> prompt>
> 
> Now the last line with a newline contains a prompt. The actual extra
> trailing prompt is then pushed to the next command execution:
> prompt_from_command1> command2\n
> <command2 output>\n
> <prompt_from_command2>
> 
> Maybe you already knew this but maybe we could archive this and point
> to it in case anyone asks. :-)

I think the confusion is the actual end meaning of this. So this is just 
so that the log for the next command starts with `testpmd> `... as 
otherwise it'd be consumed by the previous one.

One of the original reasons for reporting the logging improvements 
Bugzilla ticket 1361 was that perhaps we could provide the user with a 
the full stdout/err buffer piped into a file without any other DTS 
details. As I thought that for debugging reasons it would have been 
easier to read. Thus avoiding this kind of trickery. And the 10 lines as 
you mentioned in the ticket could just be the contents of the buffer.

>>> One thing we could improve though is removing the distribution welcome
>>> message from logs, or at least separate it from the first command sent
>>> with the interactive shell. The second option will allow us to see
>>> clearly that an interactive session has been established, although we
>>> could just emit a shorter log (something like "Started a testpmd
>>> session" and then flush the welcome screen output).
>>
>> I am not sure what you are referring to exactly, could you also expand
>> more on this please?
>>
> 
> Let's look at logs. The first thing to note is we don't log when we
> start the interactive session. The first log from the session is the
> output from the first command, such as testpmd:
> 2024/04/11 13:29:27 - test_suite - dts.SUT 1 - INFO - Sending: 'sudo
> -- sh -c '/tmp/dpdk-22.07/x86_64-linux-native-gcc/app/dpdk-testpmd -l
> 1-2 -n 4 --file-prefix=dpdk_71695_20240411132902 -a 0000:07:00.0 -a
> 0000:08:00.0  -- -i --nb-cores=1 --port-topology=paired --numa
> --tx-ip=198.18.0.1,198.18.0.2 --tx-udp=9 --forward-mode=io
> --hairpin-mode=0x0 --rx-offloads=0x0 --rx-mq-mode=0x7
> --tx-offloads=0x0 --mask-event=intr_lsc --no-mlockall''
> 2024/04/11 13:29:28 - test_suite - dts.SUT 1 - DEBUG - Got output:
> Welcome to Ubuntu 22.04.2 LTS (GNU/Linux 5.15.0-97-generic x86_64)^M
> <the welcome screen continues>
> Last login: Thu Apr 11 11:24:44 2024 from 192.168.122.1^M^M
> sudo -- sh -c '/tmp/dpdk-22.07/x86_64-linux-native-gcc/app/dpdk-testpmd
> -l 1-2 -n 4 --file-prefix=dpdk_71695_20240411132902 -a 0000:07:00.0 -a
> 0000:08:00.0  -- -i --nb-cores=1 --port-topology=paired --numa
> --tx-ip=198.18.0.1,198.18.0.2 --tx-udp=9 --forward-mode=io
> --hairpin-mode=0x0 --rx-offloads=0x0 --rx-mq-mode=0x7
> --tx-offloads=0x0 --mask-event=intr_lsc --no-mlockall'^M
> ^M
> ^[[?2004hjlinkes@dts-sut:~$ sudo -- sh -c
> '/tmp/dpdk-22.07/x86_64-linux-native-gcc/app/dpdk-testpmd -l 1-2 -n 4
> --file-prefix=dpdk_71695_20240411132902 -a 0000:07:00.0 -a
> 0000:08:00.0  -- -i --nb-cores=1 --port-topology=paired --numa
> --tx-ip=198.18.0.1,198.18.0.2 --tx-udp=9 --forward-mode=io
> --hairpin-mode=0x0 --rx-offloads=0x0 --rx-mq-mode=0x7
> --tx-offloads=0x0 --mask-event=intr_lsc --no-mlockall'^M
> ^[[?2004l^M^M
> EAL: Detected CPU lcores: 4^M
> <tespmd startup continues>
> Done^M
> testpmd> ^M^M
> 
> 2024/04/11 13:29:28 - test_suite - dts.SUT 1 - INFO - Sending: 'show
> device info all'
> 
> What we see is the testpmd command being sent and then the output is:
> Welcome screen
> Testpmd command we've sent
> Testpmd command echoed with the os prompt
> Testpmd output
> The next command
> 
> What's extra is the welcome screen with the first command we've sent.
> We don't capture any output right after establishing the channel so it
> all stays there to be captured by the first command we send.

Oh, I see. Didn't even realise this gets captured!

>> Given it's not particularly explained, I thought having two command
>> prompts (especially a trailing one) was an error. The main reason behind
>> this is that when we go to parse the port info, the last entry which is
>> "device private info" appears to be open ended, and I couldn't gather
>> much information from the testpmd source code. So I opted to parse
>> everything until the end. With a trailing command prompt this meant
>> that: device_private_info="....testpmd> ".
> 
> So the command output doesn't end with a newline? What's the exact
> output that's captured?

The command output does end with a new line, the problem is that the 
last data entry may or may not be a multi-line one, and I have no way to 
delimit it. So the current behaviour is capture everything until the 
end, and when the end is ` testpmd> `, this becomes part of it. Example 
of actual output:

   ********************* Infos for port 0  *********************
   <snip>
   Device error handling mode: none
   Device private info:
     none
   testpmd>

Here `Device error handling mode` is printed as if it's on a 
single-line, so I use \n as delimiter. Whereas `Device private info` 
just goes on a new line and if unset it prints `none`, otherwise it 
prints whatever the device has to print. Given I cannot assume how 
assume how the format will be (I don't even have examples available)... 
just parse till the end.

Re: [PATCH 1/5] dts: fix InteractiveShell command prompt filtering

[Juraj Linkeš]

On Wed, Apr 17, 2024 at 4:17 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 17/04/2024 14:06, Juraj Linkeš wrote:
> > Actually, filtering out the last prompt is fine. I got this mixed up
> > with processing the last prompt. We must process the last prompt, but
> > we don't need to print it (and there's not much reason to do so).
>
> Yeah, my confusion was about displaying the last prompt indeed. I
> recognise we need to process it to use it as an end reading symbol.
>
> > And the reason we must process it is because we're consuming the
> > output line by line. A line appears in (or is yielded by) self._stdout
> > only after a newline appears in the output. For a typical command
> > output:
> > prompt> command\n
> > <command output>\n
> > prompt>
> >
> > The last line in self._stdout is the last line of <command output>
> > because that's the last line with a newline. We send
> > _command_extra_chars to force another prompt into the output:
> > prompt> command\n
> > <command output>\n
> > prompt>\n
> > prompt>
> >
> > Now the last line with a newline contains a prompt. The actual extra
> > trailing prompt is then pushed to the next command execution:
> > prompt_from_command1> command2\n
> > <command2 output>\n
> > <prompt_from_command2>
> >
> > Maybe you already knew this but maybe we could archive this and point
> > to it in case anyone asks. :-)
>
> I think the confusion is the actual end meaning of this. So this is just
> so that the log for the next command starts with `testpmd> `... as
> otherwise it'd be consumed by the previous one.
>
> One of the original reasons for reporting the logging improvements
> Bugzilla ticket 1361 was that perhaps we could provide the user with a
> the full stdout/err buffer piped into a file without any other DTS
> details. As I thought that for debugging reasons it would have been
> easier to read. Thus avoiding this kind of trickery. And the 10 lines as
> you mentioned in the ticket could just be the contents of the buffer.
>

I mentioned the last 10 executed commands, not lines, so that's
different. The idea with the full stdout/err piped into a file sounds
very interesting, that could be helpful for debugging.

> >>> One thing we could improve though is removing the distribution welcome
> >>> message from logs, or at least separate it from the first command sent
> >>> with the interactive shell. The second option will allow us to see
> >>> clearly that an interactive session has been established, although we
> >>> could just emit a shorter log (something like "Started a testpmd
> >>> session" and then flush the welcome screen output).
> >>
> >> I am not sure what you are referring to exactly, could you also expand
> >> more on this please?
> >>
> >
> > Let's look at logs. The first thing to note is we don't log when we
> > start the interactive session. The first log from the session is the
> > output from the first command, such as testpmd:
> > 2024/04/11 13:29:27 - test_suite - dts.SUT 1 - INFO - Sending: 'sudo
> > -- sh -c '/tmp/dpdk-22.07/x86_64-linux-native-gcc/app/dpdk-testpmd -l
> > 1-2 -n 4 --file-prefix=dpdk_71695_20240411132902 -a 0000:07:00.0 -a
> > 0000:08:00.0  -- -i --nb-cores=1 --port-topology=paired --numa
> > --tx-ip=198.18.0.1,198.18.0.2 --tx-udp=9 --forward-mode=io
> > --hairpin-mode=0x0 --rx-offloads=0x0 --rx-mq-mode=0x7
> > --tx-offloads=0x0 --mask-event=intr_lsc --no-mlockall''
> > 2024/04/11 13:29:28 - test_suite - dts.SUT 1 - DEBUG - Got output:
> > Welcome to Ubuntu 22.04.2 LTS (GNU/Linux 5.15.0-97-generic x86_64)^M
> > <the welcome screen continues>
> > Last login: Thu Apr 11 11:24:44 2024 from 192.168.122.1^M^M
> > sudo -- sh -c '/tmp/dpdk-22.07/x86_64-linux-native-gcc/app/dpdk-testpmd
> > -l 1-2 -n 4 --file-prefix=dpdk_71695_20240411132902 -a 0000:07:00.0 -a
> > 0000:08:00.0  -- -i --nb-cores=1 --port-topology=paired --numa
> > --tx-ip=198.18.0.1,198.18.0.2 --tx-udp=9 --forward-mode=io
> > --hairpin-mode=0x0 --rx-offloads=0x0 --rx-mq-mode=0x7
> > --tx-offloads=0x0 --mask-event=intr_lsc --no-mlockall'^M
> > ^M
> > ^[[?2004hjlinkes@dts-sut:~$ sudo -- sh -c
> > '/tmp/dpdk-22.07/x86_64-linux-native-gcc/app/dpdk-testpmd -l 1-2 -n 4
> > --file-prefix=dpdk_71695_20240411132902 -a 0000:07:00.0 -a
> > 0000:08:00.0  -- -i --nb-cores=1 --port-topology=paired --numa
> > --tx-ip=198.18.0.1,198.18.0.2 --tx-udp=9 --forward-mode=io
> > --hairpin-mode=0x0 --rx-offloads=0x0 --rx-mq-mode=0x7
> > --tx-offloads=0x0 --mask-event=intr_lsc --no-mlockall'^M
> > ^[[?2004l^M^M
> > EAL: Detected CPU lcores: 4^M
> > <tespmd startup continues>
> > Done^M
> > testpmd> ^M^M
> >
> > 2024/04/11 13:29:28 - test_suite - dts.SUT 1 - INFO - Sending: 'show
> > device info all'
> >
> > What we see is the testpmd command being sent and then the output is:
> > Welcome screen
> > Testpmd command we've sent
> > Testpmd command echoed with the os prompt
> > Testpmd output
> > The next command
> >
> > What's extra is the welcome screen with the first command we've sent.
> > We don't capture any output right after establishing the channel so it
> > all stays there to be captured by the first command we send.
>
> Oh, I see. Didn't even realise this gets captured!
>
> >> Given it's not particularly explained, I thought having two command
> >> prompts (especially a trailing one) was an error. The main reason behind
> >> this is that when we go to parse the port info, the last entry which is
> >> "device private info" appears to be open ended, and I couldn't gather
> >> much information from the testpmd source code. So I opted to parse
> >> everything until the end. With a trailing command prompt this meant
> >> that: device_private_info="....testpmd> ".
> >
> > So the command output doesn't end with a newline? What's the exact
> > output that's captured?
>
> The command output does end with a new line, the problem is that the
> last data entry may or may not be a multi-line one, and I have no way to
> delimit it. So the current behaviour is capture everything until the
> end, and when the end is ` testpmd> `, this becomes part of it. Example
> of actual output:
>
>    ********************* Infos for port 0  *********************
>    <snip>
>    Device error handling mode: none
>    Device private info:
>      none
>    testpmd>
>
> Here `Device error handling mode` is printed as if it's on a
> single-line, so I use \n as delimiter. Whereas `Device private info`
> just goes on a new line and if unset it prints `none`, otherwise it
> prints whatever the device has to print. Given I cannot assume how
> assume how the format will be (I don't even have examples available)...
> just parse till the end.
>

Ok this makes sense now. Let's definitely drop the last prompt, there
aren't any upsides (that I can see) and this sort of downside may
occur elsewhere.

Re: [PATCH 1/5] dts: fix InteractiveShell command prompt filtering

[Jeremy Spewock]

On Thu, Apr 18, 2024 at 2:31 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
<snip>
>
> Ok this makes sense now. Let's definitely drop the last prompt, there
> aren't any upsides (that I can see) and this sort of downside may
> occur elsewhere.
+1

[PATCH 2/5] dts: skip first line of send_command output

[Luca Vizzarro]

The first line of the InteractiveShell send_command method is generally
the command input field. This sometimes is unwanted, therefore this
commit enables the possibility of omitting the first line from the
returned output.

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
---
 dts/framework/remote_session/interactive_shell.py | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index 8a9bf96ea9..e290a083e9 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -105,7 +105,9 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
             start_command = get_privileged_command(start_command)
         self.send_command(start_command)
 
-    def send_command(self, command: str, prompt: str | None = None) -> str:
+    def send_command(
+        self, command: str, prompt: str | None = None, skip_first_line: bool = False
+    ) -> str:
         """Send `command` and get all output before the expected ending string.
 
         Lines that expect input are not included in the stdout buffer, so they cannot
@@ -121,6 +123,7 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
             command: The command to send.
             prompt: After sending the command, `send_command` will be expecting this string.
                 If :data:`None`, will use the class's default prompt.
+            skip_first_line: Skip the first line when capturing the output.
 
         Returns:
             All output in the buffer before expected string.
@@ -132,6 +135,9 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
         self._stdin.flush()
         out: str = ""
         for line in self._stdout:
+            if skip_first_line:
+                skip_first_line = False
+                continue
             if prompt in line and not line.rstrip().endswith(
                 command.rstrip()
             ):  # ignore line that sent command
-- 
2.34.1

Re: [PATCH 2/5] dts: skip first line of send_command output

[Juraj Linkeš]

On Fri, Apr 12, 2024 at 1:11 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> The first line of the InteractiveShell send_command method is generally
> the command input field. This sometimes is unwanted, therefore this
> commit enables the possibility of omitting the first line from the
> returned output.
>

Oh, the first commit message was confusing. It said leading prompt
which I understood to be the first prompt (the one with the command).
I see that this commit actually addresses what I thought the first
commit was trying to do.

> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
> ---
>  dts/framework/remote_session/interactive_shell.py | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
> index 8a9bf96ea9..e290a083e9 100644
> --- a/dts/framework/remote_session/interactive_shell.py
> +++ b/dts/framework/remote_session/interactive_shell.py
> @@ -105,7 +105,9 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
>              start_command = get_privileged_command(start_command)
>          self.send_command(start_command)
>
> -    def send_command(self, command: str, prompt: str | None = None) -> str:
> +    def send_command(
> +        self, command: str, prompt: str | None = None, skip_first_line: bool = False

Do we generally want or don't want to include the first line? When do
we absolutely not want to include it?

> +    ) -> str:
>          """Send `command` and get all output before the expected ending string.
>
>          Lines that expect input are not included in the stdout buffer, so they cannot
> @@ -121,6 +123,7 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
>              command: The command to send.
>              prompt: After sending the command, `send_command` will be expecting this string.
>                  If :data:`None`, will use the class's default prompt.
> +            skip_first_line: Skip the first line when capturing the output.
>
>          Returns:
>              All output in the buffer before expected string.
> @@ -132,6 +135,9 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
>          self._stdin.flush()
>          out: str = ""
>          for line in self._stdout:
> +            if skip_first_line:
> +                skip_first_line = False
> +                continue

Is there ever a reason to distinguish between the first line and the
line with the command on it?

>              if prompt in line and not line.rstrip().endswith(
>                  command.rstrip()
>              ):  # ignore line that sent command
> --
> 2.34.1
>

Re: [PATCH 2/5] dts: skip first line of send_command output

[Luca Vizzarro]

On 16/04/2024 09:48, Juraj Linkeš wrote:
> Oh, the first commit message was confusing. It said leading prompt
> which I understood to be the first prompt (the one with the command).
> I see that this commit actually addresses what I thought the first
> commit was trying to do.

Yes, my bad!

>> -    def send_command(self, command: str, prompt: str | None = None) -> str:
>> +    def send_command(
>> +        self, command: str, prompt: str | None = None, skip_first_line: bool = False
> 
> Do we generally want or don't want to include the first line? When do
> we absolutely not want to include it?

In the case of `show port info/stats {x}` if the provided port is 
invalid, then the first message starts with `Invalid port`. By providing 
an output that skips the command prompt, this is easily checked with 
output.startswith("Invalid port") as you may have noticed in the next 
commit. Otherwise it'd be a bit more complicated. Personally, I am not 
sure whether we care about the first line. With my limited knowledge I 
don't see a reason to include it (just as much as the trailing prompt).

>> +    ) -> str:
>>           """Send `command` and get all output before the expected ending string.
>>
>>           Lines that expect input are not included in the stdout buffer, so they cannot
>> @@ -121,6 +123,7 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
>>               command: The command to send.
>>               prompt: After sending the command, `send_command` will be expecting this string.
>>                   If :data:`None`, will use the class's default prompt.
>> +            skip_first_line: Skip the first line when capturing the output.
>>
>>           Returns:
>>               All output in the buffer before expected string.
>> @@ -132,6 +135,9 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
>>           self._stdin.flush()
>>           out: str = ""
>>           for line in self._stdout:
>> +            if skip_first_line:
>> +                skip_first_line = False
>> +                continue
> 
> Is there ever a reason to distinguish between the first line and the
> line with the command on it?

As above, not really sure. Would this always be a command prompt? The 
doubt arises only because I don't understand why we'd need the command 
prompt fed back.

> 
>>               if prompt in line and not line.rstrip().endswith(
>>                   command.rstrip()
>>               ):  # ignore line that sent command
>> --
>> 2.34.1
>>

Re: [PATCH 2/5] dts: skip first line of send_command output

[Juraj Linkeš]

On Tue, Apr 16, 2024 at 2:15 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 16/04/2024 09:48, Juraj Linkeš wrote:
> > Oh, the first commit message was confusing. It said leading prompt
> > which I understood to be the first prompt (the one with the command).
> > I see that this commit actually addresses what I thought the first
> > commit was trying to do.
>
> Yes, my bad!
>
> >> -    def send_command(self, command: str, prompt: str | None = None) -> str:
> >> +    def send_command(
> >> +        self, command: str, prompt: str | None = None, skip_first_line: bool = False
> >
> > Do we generally want or don't want to include the first line? When do
> > we absolutely not want to include it?
>
> In the case of `show port info/stats {x}` if the provided port is
> invalid, then the first message starts with `Invalid port`. By providing
> an output that skips the command prompt, this is easily checked with
> output.startswith("Invalid port") as you may have noticed in the next
> commit. Otherwise it'd be a bit more complicated. Personally, I am not
> sure whether we care about the first line. With my limited knowledge I
> don't see a reason to include it (just as much as the trailing prompt).
>
> >> +    ) -> str:
> >>           """Send `command` and get all output before the expected ending string.
> >>
> >>           Lines that expect input are not included in the stdout buffer, so they cannot
> >> @@ -121,6 +123,7 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
> >>               command: The command to send.
> >>               prompt: After sending the command, `send_command` will be expecting this string.
> >>                   If :data:`None`, will use the class's default prompt.
> >> +            skip_first_line: Skip the first line when capturing the output.
> >>
> >>           Returns:
> >>               All output in the buffer before expected string.
> >> @@ -132,6 +135,9 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
> >>           self._stdin.flush()
> >>           out: str = ""
> >>           for line in self._stdout:
> >> +            if skip_first_line:
> >> +                skip_first_line = False
> >> +                continue
> >
> > Is there ever a reason to distinguish between the first line and the
> > line with the command on it?
>
> As above, not really sure. Would this always be a command prompt? The
> doubt arises only because I don't understand why we'd need the command
> prompt fed back.
>

The only thing I could think of is debugging. Maybe it could offer
some extra insight in some corner cases.

> >
> >>               if prompt in line and not line.rstrip().endswith(
> >>                   command.rstrip()
> >>               ):  # ignore line that sent command
> >> --
> >> 2.34.1
> >>
>

Re: [PATCH 2/5] dts: skip first line of send_command output

[Jeremy Spewock]

Apologies for the complications that this interactive shell provides
here. These problems didn't arise previously primarily because the
interactive shells were designed to receive commands, give you the raw
output, and then the developer extract specifically what they want
from the output and ignore the things they don't. I understand however
that in your case it might be beneficial to just consume everything.
Some of these changes seem universally good and overall not harmful to
include (like removing the trailing prompt, I can't really see a use
for it) but some others come with caveats, so if it is too complicated
this might be better to handle as something testpmd specific, and
leave the generic interactive shell to always just give you raw
output.

On Wed, Apr 17, 2024 at 9:18 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
<snip>
> > >> @@ -132,6 +135,9 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
> > >>           self._stdin.flush()
> > >>           out: str = ""
> > >>           for line in self._stdout:
> > >> +            if skip_first_line:
> > >> +                skip_first_line = False
> > >> +                continue
> > >
> > > Is there ever a reason to distinguish between the first line and the
> > > line with the command on it?
> >
> > As above, not really sure. Would this always be a command prompt? The

Whether this first line is always the command prompt or not is
specific to the shell unfortunately. In "bash-like" shells where
commands you send are echoed into stdout for easy-of-use for the
developer (like testpmd), this first line will always be the command
you sent to it. It technically isn't always required for this to
happen however, so we could make this assumption, but it could be
slightly more limiting down the line.

> > doubt arises only because I don't understand why we'd need the command
> > prompt fed back.
> >
>
> The only thing I could think of is debugging. Maybe it could offer
> some extra insight in some corner cases.

I agree that it is useful for debugging, but we do also log it
separately before sending the command. I don't think the command could
change by simply being sent into the shell unless something strange
happens like the shell breaks it across multiple lines. I think it
would be fine to exclude it, but as mentioned, it isn't always safe to
do so.

>
> > >
> > >>               if prompt in line and not line.rstrip().endswith(
> > >>                   command.rstrip()
> > >>               ):  # ignore line that sent command
> > >> --
> > >> 2.34.1
> > >>
> >

[PATCH 3/5] dts: add parsing utility module

[Luca Vizzarro]

Adds parsing text into a custom data structure. It provides a new
`TextParser` dataclass to be inherited. This implements the `parse`
method, which combined with the parser functions, it can automatically
parse the value for each field.

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
---
 dts/framework/parser.py | 147 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 147 insertions(+)
 create mode 100644 dts/framework/parser.py

diff --git a/dts/framework/parser.py b/dts/framework/parser.py
new file mode 100644
index 0000000000..5a2ba0c93a
--- /dev/null
+++ b/dts/framework/parser.py
@@ -0,0 +1,147 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2024 Arm Limited
+
+"""Parsing utility module.
+
+This module provides :class:`~TextParser` which can be used to model any data structure
+that can parse a block of text.
+"""
+
+from dataclasses import dataclass, fields, MISSING
+import re
+from typing import TypeVar
+from typing_extensions import Self
+
+T = TypeVar("T")
+
+
+META_PARSERS = "parsers"
+
+
+def chain(parser, metadata):
+    """Chain a parser function.
+
+    The parser function can take and return a single argument of any type. It is
+    up to the user to ensure that the chained functions have compatible types.
+
+    Args:
+        parser: the parser function pointer
+        metadata: pre-existing metadata to chain if any
+    """
+    parsers = metadata.get(META_PARSERS) or []
+    parsers.append(parser)
+    return {**metadata, META_PARSERS: parsers}
+
+
+def to_int(metadata={}, base=0):
+    """Converts a string to an integer.
+
+    Args:
+        metadata: pre-existing metadata to chain if any
+        base: argument passed to the constructor of ``int``
+    """
+    return chain(lambda v: int(v, base), metadata)
+
+
+def eq(v2, metadata={}):
+    """Compares two values and returns a boolean.
+
+    Args:
+        v2: value to compare with the incoming value
+        metadata: pre-existing metadata to chain if any
+    """
+    return chain(lambda v1: v1 == v2, metadata)
+
+
+def to_bool(metadata={}):
+    """Evaluates a string into a boolean.
+
+    The following case-insensitive words yield ``True``: on, yes, enabled, true.
+
+    Args:
+        metadata: pre-existing metadata to chain if any
+    """
+    return chain(lambda s: s.lower() in ["on", "yes", "enabled", "true"], metadata)
+
+
+def regex(
+    pattern: str | re.Pattern[str],
+    flags: re.RegexFlag = re.RegexFlag(0),
+    named: bool = False,
+    metadata={},
+):
+    """Searches for a regular expression in a text.
+
+    If there is only one capture group, its value is returned, otherwise a tuple containing all the
+    capture groups values is returned instead.
+
+    Args:
+        pattern: the regular expression pattern
+        flags: the regular expression flags
+        named: if set to True only the named capture groups will be returned as a dictionary
+        metadata: pre-existing metadata to chain if any
+    """
+    pattern = re.compile(pattern, flags)
+
+    def regex_parser(text: str):
+        m = pattern.search(text)
+        if m is None:
+            return m
+
+        if named:
+            return m.groupdict()
+
+        matches = m.groups()
+        if len(matches) == 1:
+            return matches[0]
+
+        return matches
+
+    return chain(regex_parser, metadata)
+
+
+@dataclass
+class TextParser:
+    """Helper abstract dataclass that parses a text according to the fields' rules.
+
+    This class is accompanied by a selection of parser functions and a generic chaining function,
+    that are to be set to the fields' metadata, to enable parsing. If a field metadata is not set with
+    any parser function, this is skipped.
+    """
+
+    @classmethod
+    def parse(cls, text: str) -> Self:
+        """The parsing class method.
+
+        This function loops through every field that has any parser function associated with it and runs
+        each parser chain to the supplied text. If a parser function returns None, it expects that parsing
+        has failed and continues to the next field.
+
+        Args:
+            text: the text to parse
+        Raises:
+            RuntimeError: if the parser did not find a match and the field does not have a default value
+                          or default factory.
+        """
+        fields_values = {}
+        for field in fields(cls):
+            parsers = field.metadata.get(META_PARSERS)
+            if parsers is None:
+                continue
+
+            field_value = text
+            for parser_fn in parsers:
+                field_value = parser_fn(field_value)
+                if field_value is None:
+                    # nothing was actually parsed, move on
+                    break
+
+            if field_value is None:
+                if field.default is MISSING and field.default_factory is MISSING:
+                    raise RuntimeError(
+                        f"parsers for field {field.name} returned None, but the field has no default"
+                    )
+            else:
+                fields_values[field.name] = field_value
+
+        return cls(**fields_values)
-- 
2.34.1

Re: [PATCH 3/5] dts: add parsing utility module

[Juraj Linkeš]

On Fri, Apr 12, 2024 at 1:11 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> Adds parsing text into a custom data structure. It provides a new
> `TextParser` dataclass to be inherited. This implements the `parse`
> method, which combined with the parser functions, it can automatically
> parse the value for each field.
>

From this commit message, I don't know why we're adding the module.
What are we going to use it for? Since you mentioned you'll send a v2,
I'll wait with review after that as I think it'll make it a bit easier
to review.

Re: [PATCH 3/5] dts: add parsing utility module

[Luca Vizzarro]

On 16/04/2024 09:59, Juraj Linkeš wrote:
>  From this commit message, I don't know why we're adding the module.
> What are we going to use it for? Since you mentioned you'll send a v2,
> I'll wait with review after that as I think it'll make it a bit easier
> to review.

Ack. Will rewrite the commit body.

Re: [PATCH 3/5] dts: add parsing utility module

[Jeremy Spewock]

On Fri, Apr 12, 2024 at 7:11 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
<snip>
> @@ -0,0 +1,147 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2024 Arm Limited
> +
> +"""Parsing utility module.
> +
> +This module provides :class:`~TextParser` which can be used to model any data structure
> +that can parse a block of text.
> +"""
> +

It would be helpful if this top level docstring explained more of how
to use the text parser and some examples of using a small dataclass
that chains some of these methods together. At first glance it wasn't
clear to me why things were done the way they were or what these
methods really provided, but looking at how you use it in the testpmd
shell made more sense.

> +from dataclasses import dataclass, fields, MISSING
> +import re
> +from typing import TypeVar
> +from typing_extensions import Self
> +
> +T = TypeVar("T")

This is declared but I don't see where it is used. There aren't many
typehints in this file, since these types can essentially be anything,
maybe using this could make some things slightly more clear (like, for
example, in eq v1 and v2 are the same type) but in most cases it's
implied so I'm not sure how beneficial this would be regardless.

> +
> +
> +META_PARSERS = "parsers"
> +
> +
> +def chain(parser, metadata):
> +    """Chain a parser function.
> +
> +    The parser function can take and return a single argument of any type. It is
> +    up to the user to ensure that the chained functions have compatible types.
> +
> +    Args:
> +        parser: the parser function pointer
> +        metadata: pre-existing metadata to chain if any
> +    """
> +    parsers = metadata.get(META_PARSERS) or []
> +    parsers.append(parser)
> +    return {**metadata, META_PARSERS: parsers}
> +
> +
> +def to_int(metadata={}, base=0):

Is it simpler to default this to base 10? I assume that's what it'll
be most of the time so we might as well allow users to skip this
parameter.

> +    """Converts a string to an integer.
> +
> +    Args:
> +        metadata: pre-existing metadata to chain if any
> +        base: argument passed to the constructor of ``int``
> +    """
> +    return chain(lambda v: int(v, base), metadata)
> +
<snip>
> +@dataclass
> +class TextParser:
> +    """Helper abstract dataclass that parses a text according to the fields' rules.
> +
> +    This class is accompanied by a selection of parser functions and a generic chaining function,
> +    that are to be set to the fields' metadata, to enable parsing. If a field metadata is not set with
> +    any parser function, this is skipped.
> +    """
> +
> +    @classmethod
> +    def parse(cls, text: str) -> Self:
> +        """The parsing class method.
> +
> +        This function loops through every field that has any parser function associated with it and runs
> +        each parser chain to the supplied text. If a parser function returns None, it expects that parsing
> +        has failed and continues to the next field.
> +
> +        Args:
> +            text: the text to parse
> +        Raises:
> +            RuntimeError: if the parser did not find a match and the field does not have a default value
> +                          or default factory.
> +        """
> +        fields_values = {}
> +        for field in fields(cls):
> +            parsers = field.metadata.get(META_PARSERS)
> +            if parsers is None:
> +                continue
> +
> +            field_value = text
> +            for parser_fn in parsers:
> +                field_value = parser_fn(field_value)
> +                if field_value is None:
> +                    # nothing was actually parsed, move on
> +                    break
> +
> +            if field_value is None:
> +                if field.default is MISSING and field.default_factory is MISSING:
> +                    raise RuntimeError(
> +                        f"parsers for field {field.name} returned None, but the field has no default"
> +                    )

If we just skip instead of raising an exception here, would this solve
the issues caused by the first and last line in the testpmd output?
The check to see if the first line is an invalid port would obviously
still not work, but would it solve the problem of the trailing prompt?

> +            else:
> +                fields_values[field.name] = field_value
> +
> +        return cls(**fields_values)
> --
> 2.34.1
>

Re: [PATCH 3/5] dts: add parsing utility module

[Luca Vizzarro]

On 29/04/2024 17:15, Jeremy Spewock wrote:
> It would be helpful if this top level docstring explained more of how
> to use the text parser and some examples of using a small dataclass
> that chains some of these methods together. At first glance it wasn't
> clear to me why things were done the way they were or what these
> methods really provided, but looking at how you use it in the testpmd
> shell made more sense.

Ack.

> This is declared but I don't see where it is used. There aren't many
> typehints in this file, since these types can essentially be anything,
> maybe using this could make some things slightly more clear (like, for
> example, in eq v1 and v2 are the same type) but in most cases it's
> implied so I'm not sure how beneficial this would be regardless.

A remnant of previous code! Well spotted.

> Is it simpler to default this to base 10? I assume that's what it'll
> be most of the time so we might as well allow users to skip this
> parameter.

Base 0 just assumes the base of the number from the number prefix[1]. So
if it's 0xAF it's base 16, 0o755 it's base 8 and so on. Of course base 
10 is assumed with no prefix. I can certainly leave it as 10 as default, 
but is there a benefit to our purpose?

> If we just skip instead of raising an exception here, would this solve
> the issues caused by the first and last line in the testpmd output?
> The check to see if the first line is an invalid port would obviously
> still not work, but would it solve the problem of the trailing prompt?

This exception is only raised when a field does not have a default value 
and there is no value to be assigned. Of course an exception is raised 
when attempting to construct and omitting a mandatory field, but this 
one is more meaningful as it provides context on what's wrong.

It is not related to neither trailing/leading strings. These don't 
affect the functionality of the class. But rather the actual parsing 
done by the "user".

[1] https://docs.python.org/3.10/library/functions.html#int

Re: [PATCH 3/5] dts: add parsing utility module

[Jeremy Spewock]

On Tue, Apr 30, 2024 at 6:49 AM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
<snip>
>
> > Is it simpler to default this to base 10? I assume that's what it'll
> > be most of the time so we might as well allow users to skip this
> > parameter.
>
> Base 0 just assumes the base of the number from the number prefix[1]. So
> if it's 0xAF it's base 16, 0o755 it's base 8 and so on. Of course base
> 10 is assumed with no prefix. I can certainly leave it as 10 as default,
> but is there a benefit to our purpose?

I wasn't aware that was the effect of having base 0. In that case, not
only is there no benefit to defaulting to base 10, I would argue there
is benefit to not doing so and leaving it 0 by default! Thanks for
letting me know!

>
> > If we just skip instead of raising an exception here, would this solve
> > the issues caused by the first and last line in the testpmd output?
> > The check to see if the first line is an invalid port would obviously
> > still not work, but would it solve the problem of the trailing prompt?
>
> This exception is only raised when a field does not have a default value
> and there is no value to be assigned. Of course an exception is raised
> when attempting to construct and omitting a mandatory field, but this
> one is more meaningful as it provides context on what's wrong.
>
> It is not related to neither trailing/leading strings. These don't
> affect the functionality of the class. But rather the actual parsing
> done by the "user".
>

Ahh, I see. I was confused, I thought the issue you were facing with
the trailing prompt might have been that you were feeding it into the
structure which uses this parsing utility and then it was throwing an
exception because it just didn't know what it was, but that makes
sense that it wouldn't even be a flag in the structure so wouldn't
have much effect. I was hoping that would make things easier so that
we could just assume that the tool using the utility will always
specify *exactly* the information it cares about and ignore anything
that it doesn't so that you could still use the raw output from
testpmd even with the irrelevant lines. I see now that this is not
relevant, apologies.

> [1] https://docs.python.org/3.10/library/functions.html#int

[PATCH 4/5] dts: add `show port info` command to TestPmdShell

[Luca Vizzarro]

Add a new TestPmdPort data structure to represent the output
returned by `show port info`, which is implemented as part of
TestPmdShell.

The TestPmdPort data structure and its derived classes are modelled
based on the relevant testpmd source code.

This implementation makes extensive use of regular expressions, which
all parse individually. The rationale behind this is to lower the risk
of the testpmd output changing as part of development. Therefore
minimising breakage.

Bugzilla ID: 1407

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
---
 dts/framework/remote_session/testpmd_shell.py | 473 +++++++++++++++++-
 1 file changed, 472 insertions(+), 1 deletion(-)

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index cb2ab6bd00..3cf123ff57 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright(c) 2023 University of New Hampshire
 # Copyright(c) 2023 PANTHEON.tech s.r.o.
+# Copyright(c) 2024 Arm Limited
 
 """Testpmd interactive shell.
 
@@ -15,14 +16,19 @@
     testpmd_shell.close()
 """
 
+from dataclasses import dataclass, field
+import re
 import time
-from enum import auto
+from enum import Flag, auto
 from pathlib import PurePath
 from typing import Callable, ClassVar
+from typing_extensions import Self
 
 from framework.exception import InteractiveCommandExecutionError
 from framework.settings import SETTINGS
 from framework.utils import StrEnum
+from framework import parser
+from framework.parser import TextParser
 
 from .interactive_shell import InteractiveShell
 
@@ -80,6 +86,439 @@ class TestPmdForwardingModes(StrEnum):
     recycle_mbufs = auto()
 
 
+class VLANOffloadFlag(Flag):
+    #:
+    STRIP = auto()
+    #:
+    FILTER = auto()
+    #:
+    EXTEND = auto()
+    #:
+    QINQ_STRIP = auto()
+
+    @classmethod
+    def from_str_dict(cls, d):
+        """Makes an instance from a dictionary containing the flag member names with an "on" value."""
+        flag = cls(0)
+        for name in cls.__members__:
+            if d.get(name) == "on":
+                flag |= cls[name]
+        return flag
+
+
+class RSSOffloadTypesFlag(Flag):
+    #:
+    ipv4 = auto()
+    #:
+    ipv4_frag = auto()
+    #:
+    ipv4_tcp = auto()
+    #:
+    ipv4_udp = auto()
+    #:
+    ipv4_sctp = auto()
+    #:
+    ipv4_other = auto()
+    #:
+    ipv6 = auto()
+    #:
+    ipv6_frag = auto()
+    #:
+    ipv6_tcp = auto()
+    #:
+    ipv6_udp = auto()
+    #:
+    ipv6_sctp = auto()
+    #:
+    ipv6_other = auto()
+    #:
+    l2_payload = auto()
+    #:
+    ipv6_ex = auto()
+    #:
+    ipv6_tcp_ex = auto()
+    #:
+    ipv6_udp_ex = auto()
+    #:
+    port = auto()
+    #:
+    vxlan = auto()
+    #:
+    geneve = auto()
+    #:
+    nvgre = auto()
+    #:
+    user_defined_22 = auto()
+    #:
+    gtpu = auto()
+    #:
+    eth = auto()
+    #:
+    s_vlan = auto()
+    #:
+    c_vlan = auto()
+    #:
+    esp = auto()
+    #:
+    ah = auto()
+    #:
+    l2tpv3 = auto()
+    #:
+    pfcp = auto()
+    #:
+    pppoe = auto()
+    #:
+    ecpri = auto()
+    #:
+    mpls = auto()
+    #:
+    ipv4_chksum = auto()
+    #:
+    l4_chksum = auto()
+    #:
+    l2tpv2 = auto()
+    #:
+    ipv6_flow_label = auto()
+    #:
+    user_defined_38 = auto()
+    #:
+    user_defined_39 = auto()
+    #:
+    user_defined_40 = auto()
+    #:
+    user_defined_41 = auto()
+    #:
+    user_defined_42 = auto()
+    #:
+    user_defined_43 = auto()
+    #:
+    user_defined_44 = auto()
+    #:
+    user_defined_45 = auto()
+    #:
+    user_defined_46 = auto()
+    #:
+    user_defined_47 = auto()
+    #:
+    user_defined_48 = auto()
+    #:
+    user_defined_49 = auto()
+    #:
+    user_defined_50 = auto()
+    #:
+    user_defined_51 = auto()
+    #:
+    l3_pre96 = auto()
+    #:
+    l3_pre64 = auto()
+    #:
+    l3_pre56 = auto()
+    #:
+    l3_pre48 = auto()
+    #:
+    l3_pre40 = auto()
+    #:
+    l3_pre32 = auto()
+    #:
+    l2_dst_only = auto()
+    #:
+    l2_src_only = auto()
+    #:
+    l4_dst_only = auto()
+    #:
+    l4_src_only = auto()
+    #:
+    l3_dst_only = auto()
+    #:
+    l3_src_only = auto()
+
+    #:
+    ip = ipv4 | ipv4_frag | ipv4_other | ipv6 | ipv6_frag | ipv6_other | ipv6_ex
+    #:
+    udp = ipv4_udp | ipv6_udp | ipv6_udp_ex
+    #:
+    tcp = ipv4_tcp | ipv6_tcp | ipv6_tcp_ex
+    #:
+    sctp = ipv4_sctp | ipv6_sctp
+    #:
+    tunnel = vxlan | geneve | nvgre
+    #:
+    vlan = s_vlan | c_vlan
+    #:
+    all = (
+        eth
+        | vlan
+        | ip
+        | tcp
+        | udp
+        | sctp
+        | l2_payload
+        | l2tpv3
+        | esp
+        | ah
+        | pfcp
+        | gtpu
+        | ecpri
+        | mpls
+        | l2tpv2
+    )
+
+    @classmethod
+    def from_list_string(cls, names: str) -> Self:
+        flag = cls(0)
+        for name in names.split():
+            flag |= cls.from_str(name)
+        return flag
+
+    @classmethod
+    def from_str(cls, name: str) -> Self:
+        member_name = name.strip().replace("-", "_")
+        return cls[member_name]
+
+    def __str__(self):
+        return self.name.replace("_", "-")
+
+
+class DeviceCapabilitiesFlag(Flag):
+    RUNTIME_RX_QUEUE_SETUP = auto()
+    """Device supports Rx queue setup after device started."""
+    RUNTIME_TX_QUEUE_SETUP = auto()
+    """Device supports Tx queue setup after device started."""
+    RXQ_SHARE = auto()
+    """Device supports shared Rx queue among ports within Rx domain and switch domain."""
+    FLOW_RULE_KEEP = auto()
+    """Device supports keeping flow rules across restart."""
+    FLOW_SHARED_OBJECT_KEEP = auto()
+    """Device supports keeping shared flow objects across restart."""
+
+
+class DeviceErrorHandlingMode(StrEnum):
+    #:
+    none = auto()
+    #:
+    passive = auto()
+    #:
+    proactive = auto()
+    #:
+    unknown = auto()
+
+
+def _validate_device_private_info(info: str) -> str | None:
+    """Ensure that we are not parsing invalid device private info output."""
+    info = info.strip()
+    if info == "none" or info.startswith("Invalid file") or info.startswith("Failed to dump"):
+        return None
+    return info
+
+
+@dataclass
+class TestPmdPort(TextParser):
+    #:
+    id: int = field(metadata=parser.to_int(parser.regex(r"Infos for port (\d+)\b")))
+    #:
+    device_name: str = field(metadata=parser.regex(r"Device name: ([^\r\n]+)"))
+    #:
+    driver_name: str = field(metadata=parser.regex(r"Driver name: ([^\r\n]+)"))
+    #:
+    socket_id: int = field(metadata=parser.to_int(parser.regex(r"Connect to socket: (\d+)")))
+    #:
+    is_link_up: bool = field(metadata=parser.eq("up", parser.regex(r"Link status: (up|down)")))
+    #:
+    link_speed: str = field(metadata=parser.regex(r"Link speed: ([^\r\n]+)"))
+    #:
+    is_link_full_duplex: bool = field(
+        metadata=parser.eq("full", parser.regex(r"Link duplex: (full|half)-duplex"))
+    )
+    #:
+    is_link_autonegotiated: bool = field(
+        metadata=parser.to_bool(parser.regex(r"Autoneg status: (On|Off)"))
+    )
+    #:
+    is_promiscuous_mode_enabled: bool = field(
+        metadata=parser.to_bool(parser.regex(r"Promiscuous mode: (enabled|disabled)"))
+    )
+    #:
+    is_allmulticast_mode_enabled: bool = field(
+        metadata=parser.to_bool(parser.regex(r"Allmulticast mode: (enabled|disabled)"))
+    )
+    #: Maximum number of MAC addresses
+    max_mac_addresses_num: int = field(
+        metadata=parser.to_int(parser.regex(r"Maximum number of MAC addresses: (\d+)"))
+    )
+    #: Maximum configurable length of RX packet
+    max_hash_mac_addresses_num: int = field(
+        metadata=parser.to_int(
+            parser.regex(r"Maximum number of MAC addresses of hash filtering: (\d+)")
+        )
+    )
+    #: Minimum size of RX buffer
+    min_rx_bufsize: int = field(
+        metadata=parser.to_int(parser.regex(r"Minimum size of RX buffer: (\d+)"))
+    )
+    #: Maximum configurable length of RX packet
+    max_rx_packet_length: int = field(
+        metadata=parser.to_int(parser.regex(r"Maximum configurable length of RX packet: (\d+)"))
+    )
+    #: Maximum configurable size of LRO aggregated packet
+    max_lro_packet_size: int = field(
+        metadata=parser.to_int(
+            parser.regex(r"Maximum configurable size of LRO aggregated packet: (\d+)")
+        )
+    )
+
+    #: Current number of RX queues
+    rx_queues_num: int = field(
+        metadata=parser.to_int(parser.regex(r"Current number of RX queues: (\d+)"))
+    )
+    #: Max possible RX queues
+    max_rx_queues_num: int = field(
+        metadata=parser.to_int(parser.regex(r"Max possible RX queues: (\d+)"))
+    )
+    #: Max possible number of RXDs per queue
+    max_queue_rxd_num: int = field(
+        metadata=parser.to_int(parser.regex(r"Max possible number of RXDs per queue: (\d+)"))
+    )
+    #: Min possible number of RXDs per queue
+    min_queue_rxd_num: int = field(
+        metadata=parser.to_int(parser.regex(r"Min possible number of RXDs per queue: (\d+)"))
+    )
+    #: RXDs number alignment
+    rxd_alignment_num: int = field(
+        metadata=parser.to_int(parser.regex(r"RXDs number alignment: (\d+)"))
+    )
+
+    #: Current number of TX queues
+    tx_queues_num: int = field(
+        metadata=parser.to_int(parser.regex(r"Current number of TX queues: (\d+)"))
+    )
+    #: Max possible TX queues
+    max_tx_queues_num: int = field(
+        metadata=parser.to_int(parser.regex(r"Max possible TX queues: (\d+)"))
+    )
+    #: Max possible number of TXDs per queue
+    max_queue_txd_num: int = field(
+        metadata=parser.to_int(parser.regex(r"Max possible number of TXDs per queue: (\d+)"))
+    )
+    #: Min possible number of TXDs per queue
+    min_queue_txd_num: int = field(
+        metadata=parser.to_int(parser.regex(r"Min possible number of TXDs per queue: (\d+)"))
+    )
+    #: TXDs number alignment
+    txd_alignment_num: int = field(
+        metadata=parser.to_int(parser.regex(r"TXDs number alignment: (\d+)"))
+    )
+    #: Max segment number per packet
+    max_packet_segment_num: int = field(
+        metadata=parser.to_int(parser.regex(r"Max segment number per packet: (\d+)"))
+    )
+    #: Max segment number per MTU/TSO
+    max_mtu_segment_num: int = field(
+        metadata=parser.to_int(parser.regex(r"Max segment number per MTU\/TSO: (\d+)"))
+    )
+
+    #:
+    device_capabilities: DeviceCapabilitiesFlag = field(
+        metadata=parser.chain(
+            DeviceCapabilitiesFlag,
+            parser.to_int(parser.regex(r"Device capabilities: (0x[A-Fa-f\d]+)")),
+        )
+    )
+    #:
+    device_error_handling_mode: DeviceErrorHandlingMode = field(
+        metadata=parser.chain(
+            DeviceErrorHandlingMode, parser.regex(r"Device error handling mode: (\w+)")
+        )
+    )
+    #:
+    device_private_info: str | None = field(
+        default=None,
+        metadata=parser.chain(
+            _validate_device_private_info,
+            parser.regex(r"Device private info:\s+([\s\S]+)", re.MULTILINE),
+        ),
+    )
+
+    #:
+    hash_key_size: int | None = field(
+        default=None, metadata=parser.to_int(parser.regex(r"Hash key size in bytes: (\d+)"))
+    )
+    #:
+    redirection_table_size: int | None = field(
+        default=None, metadata=parser.to_int(parser.regex(r"Redirection table size: (\d+)"))
+    )
+    #:
+    supported_rss_offload_flow_types: RSSOffloadTypesFlag = field(
+        default=RSSOffloadTypesFlag(0),
+        metadata=parser.chain(
+            RSSOffloadTypesFlag.from_list_string,
+            parser.regex(r"Supported RSS offload flow types:((?:\r?\n?  \S+)+)", re.MULTILINE),
+        ),
+    )
+
+    #:
+    mac_address: str | None = field(
+        default=None, metadata=parser.regex(r"MAC address: ([A-Fa-f0-9:]+)")
+    )
+    #:
+    fw_version: str | None = field(
+        default=None, metadata=parser.regex(r"Firmware-version: ([^\r\n]+)")
+    )
+    #:
+    dev_args: str | None = field(default=None, metadata=parser.regex(r"Devargs: ([^\r\n]+)"))
+    #: Socket id of the memory allocation
+    mem_alloc_socket_id: int | None = field(
+        default=None,
+        metadata=parser.to_int(parser.regex(r"memory allocation on the socket: (\d+)")),
+    )
+    #:
+    mtu: int | None = field(default=None, metadata=parser.to_int(parser.regex(r"MTU: (\d+)")))
+
+    #:
+    vlan_offload: VLANOffloadFlag | None = field(
+        default=None,
+        metadata=parser.chain(
+            VLANOffloadFlag.from_str_dict,
+            parser.regex(
+                r"VLAN offload:\s+"
+                r"strip (?P<STRIP>on|off), "
+                r"filter (?P<FILTER>on|off), "
+                r"extend (?P<EXTEND>on|off), "
+                r"qinq strip (?P<QINQ_STRIP>on|off)$",
+                re.MULTILINE,
+                named=True,
+            ),
+        ),
+    )
+
+    #: Maximum size of RX buffer
+    max_rx_bufsize: int | None = field(
+        default=None, metadata=parser.to_int(parser.regex(r"Maximum size of RX buffer: (\d+)"))
+    )
+    #: Maximum number of VFs
+    max_vfs_num: int | None = field(
+        default=None, metadata=parser.to_int(parser.regex(r"Maximum number of VFs: (\d+)"))
+    )
+    #: Maximum number of VMDq pools
+    max_vmdq_pools_num: int | None = field(
+        default=None, metadata=parser.to_int(parser.regex(r"Maximum number of VMDq pools: (\d+)"))
+    )
+
+    #:
+    switch_name: str | None = field(default=None, metadata=parser.regex(r"Switch name: ([\r\n]+)"))
+    #:
+    switch_domain_id: int | None = field(
+        default=None, metadata=parser.to_int(parser.regex(r"Switch domain Id: (\d+)"))
+    )
+    #:
+    switch_port_id: int | None = field(
+        default=None, metadata=parser.to_int(parser.regex(r"Switch Port Id: (\d+)"))
+    )
+    #:
+    switch_rx_domain: int | None = field(
+        default=None, metadata=parser.to_int(parser.regex(r"Switch Rx domain: (\d+)"))
+    )
+
+
 class TestPmdShell(InteractiveShell):
     """Testpmd interactive shell.
 
@@ -225,6 +664,38 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
                 f"Test pmd failed to set fwd mode to {mode.value}"
             )
 
+    def show_port_info_all(self) -> list[TestPmdPort]:
+        """Returns the information of all the ports."""
+        output = self.send_command("show port info all")
+
+        ports = []
+        iter = re.finditer(r"\*+.+\*+", output)
+        if next(iter, False):  # we are slicing retrospectively, skip first block
+            start_pos = 0
+            for block in iter:
+                end_pos = block.start()
+                ports.append(TestPmdPort.parse(output[start_pos:end_pos]))
+                start_pos = end_pos
+
+            ports.append(TestPmdPort.parse(output[start_pos:]))
+
+        return ports
+
+    def show_port_info(self, port_id: int) -> TestPmdPort:
+        """Returns the given port information.
+
+        Args:
+            port_id: The port ID to gather information for.
+
+        Raises:
+            InteractiveCommandExecutionError: If `port_id` is invalid.
+        """
+        output = self.send_command(f"show port info {port_id}", skip_first_line=True)
+        if output.startswith("Invalid port"):
+            raise InteractiveCommandExecutionError("invalid port given")
+
+        return TestPmdPort.parse(output)
+
     def close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
         self.send_command("quit", "")
-- 
2.34.1

Re: [PATCH 4/5] dts: add `show port info` command to TestPmdShell

[Juraj Linkeš]

On Fri, Apr 12, 2024 at 1:11 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> Add a new TestPmdPort data structure to represent the output
> returned by `show port info`, which is implemented as part of
> TestPmdShell.
>
> The TestPmdPort data structure and its derived classes are modelled
> based on the relevant testpmd source code.
>
> This implementation makes extensive use of regular expressions, which
> all parse individually. The rationale behind this is to lower the risk
> of the testpmd output changing as part of development. Therefore
> minimising breakage.
>
> Bugzilla ID: 1407
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>

<snip>

> +@dataclass
> +class TestPmdPort(TextParser):

This and the classes above are missing docstrings.

<snip>

> @@ -225,6 +664,38 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
>                  f"Test pmd failed to set fwd mode to {mode.value}"
>              )
>
> +    def show_port_info_all(self) -> list[TestPmdPort]:
> +        """Returns the information of all the ports."""

Can we add sample output so that the format of what we're trying to
parse is clear?

> +        output = self.send_command("show port info all")
> +
> +        ports = []
> +        iter = re.finditer(r"\*+.+\*+", output)
> +        if next(iter, False):  # we are slicing retrospectively, skip first block
> +            start_pos = 0
> +            for block in iter:
> +                end_pos = block.start()
> +                ports.append(TestPmdPort.parse(output[start_pos:end_pos]))
> +                start_pos = end_pos
> +
> +            ports.append(TestPmdPort.parse(output[start_pos:]))
> +
> +        return ports

Can this be done the same way it's done in the last commit?

iter = re.finditer(r"(^  #*.+#*$[^#]+)^  #*$", output, re.MULTILINE)
return [TestPmdPortStats.parse(block.group(1)) for block in iter]

Looks much better.

> +
> +    def show_port_info(self, port_id: int) -> TestPmdPort:
> +        """Returns the given port information.
> +
> +        Args:
> +            port_id: The port ID to gather information for.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `port_id` is invalid.
> +        """
> +        output = self.send_command(f"show port info {port_id}", skip_first_line=True)
> +        if output.startswith("Invalid port"):
> +            raise InteractiveCommandExecutionError("invalid port given")
> +
> +        return TestPmdPort.parse(output)
> +
>      def close(self) -> None:
>          """Overrides :meth:`~.interactive_shell.close`."""
>          self.send_command("quit", "")
> --
> 2.34.1
>

Re: [PATCH 4/5] dts: add `show port info` command to TestPmdShell

[Luca Vizzarro]

On 16/04/2024 10:03, Juraj Linkeš wrote:
>> +@dataclass
>> +class TestPmdPort(TextParser):
> 
> This and the classes above are missing docstrings.

Ack.

>> @@ -225,6 +664,38 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
>>                   f"Test pmd failed to set fwd mode to {mode.value}"
>>               )
>>
>> +    def show_port_info_all(self) -> list[TestPmdPort]:
>> +        """Returns the information of all the ports."""
> 
> Can we add sample output so that the format of what we're trying to
> parse is clear?

Ack.

>> +        output = self.send_command("show port info all")
>> +
>> +        ports = []
>> +        iter = re.finditer(r"\*+.+\*+", output)
>> +        if next(iter, False):  # we are slicing retrospectively, skip first block
>> +            start_pos = 0
>> +            for block in iter:
>> +                end_pos = block.start()
>> +                ports.append(TestPmdPort.parse(output[start_pos:end_pos]))
>> +                start_pos = end_pos
>> +
>> +            ports.append(TestPmdPort.parse(output[start_pos:]))
>> +
>> +        return ports
> 
> Can this be done the same way it's done in the last commit?
> 
> iter = re.finditer(r"(^  #*.+#*$[^#]+)^  #*$", output, re.MULTILINE)
> return [TestPmdPortStats.parse(block.group(1)) for block in iter]
> 
> Looks much better.

I agree that it looks much better. I gave it a first attempt to come up 
with a regular expression that is not too complicated and is able to 
match blocks individually. I've noticed that blocks start with:

   \n********* Infos for port X ************

but don't have an actual ending delimiter, unlike for the stats. I'll 
experiment with some look ahead constructs. The easiest solution is to 
match everything that is not * ([^*]+) but can we be certain that there 
won't be any asterisk in the actual information?

Re: [PATCH 4/5] dts: add `show port info` command to TestPmdShell

[Juraj Linkeš]

On Tue, Apr 16, 2024 at 2:24 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 16/04/2024 10:03, Juraj Linkeš wrote:
> >> +@dataclass
> >> +class TestPmdPort(TextParser):
> >
> > This and the classes above are missing docstrings.
>
> Ack.
>
> >> @@ -225,6 +664,38 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
> >>                   f"Test pmd failed to set fwd mode to {mode.value}"
> >>               )
> >>
> >> +    def show_port_info_all(self) -> list[TestPmdPort]:
> >> +        """Returns the information of all the ports."""
> >
> > Can we add sample output so that the format of what we're trying to
> > parse is clear?
>
> Ack.
>
> >> +        output = self.send_command("show port info all")
> >> +
> >> +        ports = []
> >> +        iter = re.finditer(r"\*+.+\*+", output)
> >> +        if next(iter, False):  # we are slicing retrospectively, skip first block
> >> +            start_pos = 0
> >> +            for block in iter:
> >> +                end_pos = block.start()
> >> +                ports.append(TestPmdPort.parse(output[start_pos:end_pos]))
> >> +                start_pos = end_pos
> >> +
> >> +            ports.append(TestPmdPort.parse(output[start_pos:]))
> >> +
> >> +        return ports
> >
> > Can this be done the same way it's done in the last commit?
> >
> > iter = re.finditer(r"(^  #*.+#*$[^#]+)^  #*$", output, re.MULTILINE)
> > return [TestPmdPortStats.parse(block.group(1)) for block in iter]
> >
> > Looks much better.
>
> I agree that it looks much better. I gave it a first attempt to come up
> with a regular expression that is not too complicated and is able to
> match blocks individually. I've noticed that blocks start with:
>
>    \n********* Infos for port X ************
>
> but don't have an actual ending delimiter, unlike for the stats.

Ah, so that's the difference and the reason. I guess the ending
delimiter is either the start of the next section of the prompt (or
the end of the string).

> I'll
> experiment with some look ahead constructs. The easiest solution is to
> match everything that is not * ([^*]+) but can we be certain that there
> won't be any asterisk in the actual information?

We can't. But we can be reasonably certain there won't be five
consecutive asterisks, so maybe we can work with that.

Re: [PATCH 4/5] dts: add `show port info` command to TestPmdShell

[Luca Vizzarro]

On 17/04/2024 14:22, Juraj Linkeš wrote:
>> I agree that it looks much better. I gave it a first attempt to come up
>> with a regular expression that is not too complicated and is able to
>> match blocks individually. I've noticed that blocks start with:
>>
>>     \n********* Infos for port X ************
>>
>> but don't have an actual ending delimiter, unlike for the stats.
> 
> Ah, so that's the difference and the reason. I guess the ending
> delimiter is either the start of the next section of the prompt (or
> the end of the string).

Yes, but it's not as trivial unfortunately. In the current code I am 
effectively just finding all the start positions and slice.

>> I'll
>> experiment with some look ahead constructs. The easiest solution is to
>> match everything that is not * ([^*]+) but can we be certain that there
>> won't be any asterisk in the actual information?
> 
> We can't. But we can be reasonably certain there won't be five
> consecutive asterisks, so maybe we can work with that.

We can work with that by using look ahead constructs as mentioned, which 
can be quite intensive. For example:

   /(?<=\n\*).*?(?=\n\*|$)/gs

looks for the start delimiter and for the start of the next block or the 
end. This works perfectly! But it's performing 9576 steps (!) for just 
two ports. The current solution only takes 10 steps in total.

Re: [PATCH 4/5] dts: add `show port info` command to TestPmdShell

[Luca Vizzarro]

On 17/04/2024 15:25, Luca Vizzarro wrote:
> On 17/04/2024 14:22, Juraj Linkeš wrote:
>>> I'll
>>> experiment with some look ahead constructs. The easiest solution is to
>>> match everything that is not * ([^*]+) but can we be certain that there
>>> won't be any asterisk in the actual information?
>>
>> We can't. But we can be reasonably certain there won't be five
>> consecutive asterisks, so maybe we can work with that.
> 
> We can work with that by using look ahead constructs as mentioned, which 
> can be quite intensive. For example:
> 
>    /(?<=\n\*).*?(?=\n\*|$)/gs
> 
> looks for the start delimiter and for the start of the next block or the 
> end. This works perfectly! But it's performing 9576 steps (!) for just 
> two ports. The current solution only takes 10 steps in total.

Thinking of it... we are not really aiming for performance, so I guess 
if it simplifies and it's justifiable, then it's not a problem. 
Especially since this command shouldn't be called continuosly.

The equivalent /\n\*.+?(?=\n\*|$)/gs (but slightly more optimised) takes 
approximately 3*input_length steps to run (according to regex101 at 
least). If that's reasonable enough, I can do this:

   iter = re.finditer(input, "\n\*.+?(?=\n\*|$)", re.S)
   return [TestPmdPortInfo.parse(match.group(0)) for match in iter]

Another optimization is artificially adding a `\n*` delimiter at the end 
before feeding it to the regex, thus removing the alternative case (|$), 
and making it 2*len steps:

   input += "\n*"
   iter = re.finditer(input, "\n\*.+?(?=\n\*)", re.S)
   return [TestPmdPortInfo.parse(match.group(0)) for match in iter]

Let me know what you think!

Best,
Luca

Re: [PATCH 4/5] dts: add `show port info` command to TestPmdShell

[Juraj Linkeš]

On Wed, Apr 17, 2024 at 5:29 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 17/04/2024 15:25, Luca Vizzarro wrote:
> > On 17/04/2024 14:22, Juraj Linkeš wrote:
> >>> I'll
> >>> experiment with some look ahead constructs. The easiest solution is to
> >>> match everything that is not * ([^*]+) but can we be certain that there
> >>> won't be any asterisk in the actual information?
> >>
> >> We can't. But we can be reasonably certain there won't be five
> >> consecutive asterisks, so maybe we can work with that.
> >
> > We can work with that by using look ahead constructs as mentioned, which
> > can be quite intensive. For example:
> >
> >    /(?<=\n\*).*?(?=\n\*|$)/gs
> >
> > looks for the start delimiter and for the start of the next block or the
> > end. This works perfectly! But it's performing 9576 steps (!) for just
> > two ports. The current solution only takes 10 steps in total.
>
> Thinking of it... we are not really aiming for performance, so I guess
> if it simplifies and it's justifiable, then it's not a problem.
> Especially since this command shouldn't be called continuosly.
>

We have to weigh the pros and cons on an individual basis. In this
case, the output is going to be short so basically any solution is
going to be indistinguishable from any other, performance wise.

> The equivalent /\n\*.+?(?=\n\*|$)/gs (but slightly more optimised) takes
> approximately 3*input_length steps to run (according to regex101 at
> least). If that's reasonable enough, I can do this:
>
>    iter = re.finditer(input, "\n\*.+?(?=\n\*|$)", re.S)
>    return [TestPmdPortInfo.parse(match.group(0)) for match in iter]
>
> Another optimization is artificially adding a `\n*` delimiter at the end
> before feeding it to the regex, thus removing the alternative case (|$),
> and making it 2*len steps:
>
>    input += "\n*"
>    iter = re.finditer(input, "\n\*.+?(?=\n\*)", re.S)
>    return [TestPmdPortInfo.parse(match.group(0)) for match in iter]
>

I like this second one a bit more. How does the performance change if
we try to match four asterisks "\n\****.+?(?=\n\****)"? Four asterisks
shouldn't randomly be in the output as that's basically another
delimited.

And we should document this in the docstring - sample output, then
explain the extra characters and the regex itself. We shouldn't forget
this in the other commit as well (show port stats).

> Let me know what you think!
>
> Best,
> Luca

Re: [PATCH 4/5] dts: add `show port info` command to TestPmdShell

[Luca Vizzarro]

On 18/04/2024 07:41, Juraj Linkeš wrote:
>> The equivalent /\n\*.+?(?=\n\*|$)/gs (but slightly more optimised) takes
>> approximately 3*input_length steps to run (according to regex101 at
>> least). If that's reasonable enough, I can do this:
>>
>>     iter = re.finditer(input, "\n\*.+?(?=\n\*|$)", re.S)
>>     return [TestPmdPortInfo.parse(match.group(0)) for match in iter]
>>
>> Another optimization is artificially adding a `\n*` delimiter at the end
>> before feeding it to the regex, thus removing the alternative case (|$),
>> and making it 2*len steps:
>>
>>     input += "\n*"
>>     iter = re.finditer(input, "\n\*.+?(?=\n\*)", re.S)
>>     return [TestPmdPortInfo.parse(match.group(0)) for match in iter]
>>
> 
> I like this second one a bit more. How does the performance change if
> we try to match four asterisks "\n\****.+?(?=\n\****)"? Four asterisks
> shouldn't randomly be in the output as that's basically another
> delimited.

The difference is negligible as the regex walks every character anyways 
– while there is a match. Either \* or \*{4} will result in a match. The 
problem is that if an attempt of match fails, the regex backtracks, this 
is where the problem is. Of course if we already matched 4 asterisks, 
then the backtracking can skip the whole sequence at once (compared to 
1), but it's only going to be 3 steps less per **** found. It's a bigger 
difference if we attempt to match all the asterisks. The lookahead 
construct also increases backtracking.

In the meantime, still by amending the output, I've got a solution that 
doesn't perform any look aheads:

   \*{21}.+?\n{2}

instead of treating blocks as they are (\n******<snip>\n), we can add an 
extra \n at the end and treat the blocks as: ******<snip>\n\n. Basically 
this assumes that an empty line is the end delimiter. This takes 
input_length/2 steps!

Of course in reality every \n is \r\n, as I've discovered that when 
shells are invoked using paramiko, the stream becomes CRLF for some 
reason I haven't explored. I think this was worth mentioning for 
everybody, in case surprise carriage returns may reveal disruptive.

> And we should document this in the docstring - sample output, then
> explain the extra characters and the regex itself. We shouldn't forget
> this in the other commit as well (show port stats).

Ack.

[PATCH 5/5] dts: add `show port stats` command to TestPmdShell

[Luca Vizzarro]

Add a new TestPmdPortStats data structure to represent the output
returned by `show port stats`, which is implemented as part of
TestPmdShell.

Bugzilla ID: 1407

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
---
 dts/framework/remote_session/testpmd_shell.py | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index 3cf123ff57..baf47d1a32 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -519,6 +519,42 @@ class TestPmdPort(TextParser):
     )
 
 
+@dataclass
+class TestPmdPortStats(TextParser):
+    """Port statistics."""
+
+    #:
+    port_id: int = field(metadata=parser.to_int(parser.regex(r"NIC statistics for port (\d+)")))
+
+    #:
+    rx_packets: int = field(metadata=parser.to_int(parser.regex(r"RX-packets:\s+(\d+)")))
+    #:
+    rx_missed: int = field(metadata=parser.to_int(parser.regex(r"RX-missed:\s+(\d+)")))
+    #:
+    rx_bytes: int = field(metadata=parser.to_int(parser.regex(r"RX-bytes:\s+(\d+)")))
+    #:
+    rx_errors: int = field(metadata=parser.to_int(parser.regex(r"RX-errors:\s+(\d+)")))
+    #:
+    rx_nombuf: int = field(metadata=parser.to_int(parser.regex(r"RX-nombuf:\s+(\d+)")))
+
+    #:
+    tx_packets: int = field(metadata=parser.to_int(parser.regex(r"TX-packets:\s+(\d+)")))
+    #:
+    tx_errors: int = field(metadata=parser.to_int(parser.regex(r"TX-errors:\s+(\d+)")))
+    #:
+    tx_bytes: int = field(metadata=parser.to_int(parser.regex(r"TX-bytes:\s+(\d+)")))
+
+    #:
+    rx_pps: int = field(metadata=parser.to_int(parser.regex(r"Rx-pps:\s+(\d+)")))
+    #:
+    rx_bps: int = field(metadata=parser.to_int(parser.regex(r"Rx-bps:\s+(\d+)")))
+
+    #:
+    tx_pps: int = field(metadata=parser.to_int(parser.regex(r"Tx-pps:\s+(\d+)")))
+    #:
+    tx_bps: int = field(metadata=parser.to_int(parser.regex(r"Tx-bps:\s+(\d+)")))
+
+
 class TestPmdShell(InteractiveShell):
     """Testpmd interactive shell.
 
@@ -696,6 +732,28 @@ def show_port_info(self, port_id: int) -> TestPmdPort:
 
         return TestPmdPort.parse(output)
 
+    def show_port_stats_all(self) -> list[TestPmdPortStats]:
+        """Returns the statistics of all the ports."""
+        output = self.send_command("show port stats all")
+
+        iter = re.finditer(r"(^  #*.+#*$[^#]+)^  #*$", output, re.MULTILINE)
+        return [TestPmdPortStats.parse(block.group(1)) for block in iter]
+
+    def show_port_stats(self, port_id: int) -> TestPmdPortStats:
+        """Returns the given port statistics.
+
+        Args:
+            port_id: The port ID to gather information for.
+
+        Raises:
+            InteractiveCommandExecutionError: If `port_id` is invalid.
+        """
+        output = self.send_command(f"show port stats {port_id}")
+        if output.startswith("Invalid port"):
+            raise InteractiveCommandExecutionError("invalid port given")
+
+        return TestPmdPortStats.parse(output)
+
     def close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
         self.send_command("quit", "")
-- 
2.34.1

Re: [PATCH 5/5] dts: add `show port stats` command to TestPmdShell

[Juraj Linkeš]

On Fri, Apr 12, 2024 at 1:11 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> Add a new TestPmdPortStats data structure to represent the output
> returned by `show port stats`, which is implemented as part of
> TestPmdShell.
>
> Bugzilla ID: 1407
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>

Acked-by: Juraj Linkeš <juraj.linkes@pantheon.tech>

Re: [PATCH 5/5] dts: add `show port stats` command to TestPmdShell

[Jeremy Spewock]

On Fri, Apr 12, 2024 at 7:11 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
<snip>
> +    def show_port_stats(self, port_id: int) -> TestPmdPortStats:
> +        """Returns the given port statistics.
> +
> +        Args:
> +            port_id: The port ID to gather information for.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `port_id` is invalid.
> +        """
> +        output = self.send_command(f"show port stats {port_id}")

Does this also need to skip the first line in the output?

> +        if output.startswith("Invalid port"):
> +            raise InteractiveCommandExecutionError("invalid port given")
> +
> +        return TestPmdPortStats.parse(output)
> +
>      def close(self) -> None:
>          """Overrides :meth:`~.interactive_shell.close`."""
>          self.send_command("quit", "")
> --
> 2.34.1
>

Re: [PATCH 5/5] dts: add `show port stats` command to TestPmdShell

[Luca Vizzarro]

On 29/04/2024 16:54, Jeremy Spewock wrote:
> On Fri, Apr 12, 2024 at 7:11 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>> +        output = self.send_command(f"show port stats {port_id}")
> 
> Does this also need to skip the first line in the output?

Yep, well spotted!

[PATCH v2 0/5] dts: testpmd show port info/stats

[Luca Vizzarro]

Hello,

sending in v2:
- refactored parsing utility
- changed functionality of the parser for conciseness
- added a usage example to the parsing module

Best,
Luca

---
Depends-on: series-31896 ("dts: update mypy and clean up")
---

Luca Vizzarro (5):
  dts: fix InteractiveShell command prompt filtering
  dts: skip first line of send command output
  dts: add parsing utility module
  dts: add `show port info` command to TestPmdShell
  dts: add `show port stats` command to TestPmdShell

 dts/framework/parser.py                       | 199 +++++++
 .../remote_session/interactive_shell.py       |  10 +-
 dts/framework/remote_session/testpmd_shell.py | 558 +++++++++++++++++-
 3 files changed, 764 insertions(+), 3 deletions(-)
 create mode 100644 dts/framework/parser.py

-- 
2.34.1

[PATCH v2 1/5] dts: fix InteractiveShell command prompt filtering

[Luca Vizzarro]

When sending a command using an instance of InteractiveShell the output
should filter out the trailing shell prompt when returning it. After
every command two shell prompts are summoned. One is consumed as it is
used as a delimiter for the command output. The second one is not
consumed and left for the next command to be sent.

Given that the consumed prompt is merely a delimiter, this should not be
added to the returned output, as it may be mistakenly be interpreted as
the command's own output.

Bugzilla ID: 1411
Fixes: 88489c0501af ("dts: add smoke tests")

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
---
 dts/framework/remote_session/interactive_shell.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index 074a541279..aa5d2d9be8 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -132,11 +132,11 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
         self._stdin.flush()
         out: str = ""
         for line in self._stdout:
-            out += line
             if prompt in line and not line.rstrip().endswith(
                 command.rstrip()
             ):  # ignore line that sent command
                 break
+            out += line
         self._logger.debug(f"Got output: {out}")
         return out
 
-- 
2.34.1

Re: [PATCH v2 1/5] dts: fix InteractiveShell command prompt filtering

[Jeremy Spewock]

On Thu, May 9, 2024 at 7:26 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> When sending a command using an instance of InteractiveShell the output
> should filter out the trailing shell prompt when returning it. After
> every command two shell prompts are summoned. One is consumed as it is
> used as a delimiter for the command output. The second one is not
> consumed and left for the next command to be sent.
>
> Given that the consumed prompt is merely a delimiter, this should not be
> added to the returned output, as it may be mistakenly be interpreted as
> the command's own output.
>
> Bugzilla ID: 1411
> Fixes: 88489c0501af ("dts: add smoke tests")
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> ---
>  dts/framework/remote_session/interactive_shell.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
Reviewed-by: Jeremy Spewock <jspewock@iol.unh.edu>

[PATCH v2 2/5] dts: skip first line of send command output

[Luca Vizzarro]

The first line of the InteractiveShell send_command method is generally
the command input field. This sometimes is unwanted, therefore this
commit enables the possibility of omitting the first line from the
returned output.

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
---
 dts/framework/remote_session/interactive_shell.py | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/dts/framework/remote_session/interactive_shell.py b/dts/framework/remote_session/interactive_shell.py
index aa5d2d9be8..c025c52ba3 100644
--- a/dts/framework/remote_session/interactive_shell.py
+++ b/dts/framework/remote_session/interactive_shell.py
@@ -105,7 +105,9 @@ def _start_application(self, get_privileged_command: Callable[[str], str] | None
             start_command = get_privileged_command(start_command)
         self.send_command(start_command)
 
-    def send_command(self, command: str, prompt: str | None = None) -> str:
+    def send_command(
+        self, command: str, prompt: str | None = None, skip_first_line: bool = False
+    ) -> str:
         """Send `command` and get all output before the expected ending string.
 
         Lines that expect input are not included in the stdout buffer, so they cannot
@@ -121,6 +123,7 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
             command: The command to send.
             prompt: After sending the command, `send_command` will be expecting this string.
                 If :data:`None`, will use the class's default prompt.
+            skip_first_line: Skip the first line when capturing the output.
 
         Returns:
             All output in the buffer before expected string.
@@ -132,6 +135,9 @@ def send_command(self, command: str, prompt: str | None = None) -> str:
         self._stdin.flush()
         out: str = ""
         for line in self._stdout:
+            if skip_first_line:
+                skip_first_line = False
+                continue
             if prompt in line and not line.rstrip().endswith(
                 command.rstrip()
             ):  # ignore line that sent command
-- 
2.34.1

Re: [PATCH v2 2/5] dts: skip first line of send command output

[Jeremy Spewock]

On Thu, May 9, 2024 at 7:26 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> The first line of the InteractiveShell send_command method is generally
> the command input field. This sometimes is unwanted, therefore this
> commit enables the possibility of omitting the first line from the
> returned output.
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> ---

Reviewed-by: Jeremy Spewock <jspewock@iol.unh.edu>

[PATCH v2 3/5] dts: add parsing utility module

[Luca Vizzarro]

Adds parsing text into a custom dataclass. It provides a new
`TextParser` dataclass to be inherited. This implements the `parse`
method, which combined with the parser functions, it can automatically
parse the value for each field.

This new utility will facilitate and simplify the parsing of complex
command outputs, while ensuring that the codebase does not get bloated
and stays flexible.

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
---
 dts/framework/parser.py | 199 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 199 insertions(+)
 create mode 100644 dts/framework/parser.py

diff --git a/dts/framework/parser.py b/dts/framework/parser.py
new file mode 100644
index 0000000000..5b4acddead
--- /dev/null
+++ b/dts/framework/parser.py
@@ -0,0 +1,199 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2024 Arm Limited
+
+r"""Parsing utility module.
+
+This module provides :class:`~TextParser` which can be used to model any dataclass to a block of
+text.
+
+Usage example::
+..code:: python
+
+    from dataclasses import dataclass, field
+    from enum import Enum
+    from framework.parser import TextParser
+
+    class Colour(Enum):
+        BLACK = 1
+        WHITE = 2
+
+        @classmethod
+        def from_str(cls, text: str):
+            match text:
+                case "black":
+                    return cls.BLACK
+                case "white":
+                    return cls.WHITE
+                case _:
+                    return None # unsupported colour
+
+        @classmethod
+        def make_parser(cls):
+            # make a parser function that finds a match and
+            # then makes it a Colour object through Colour.from_str
+            return TextParser.compose(cls.from_str, TextParser.find(r"is a (\w+)"))
+
+    @dataclass
+    class Animal(TextParser):
+        kind: str = field(metadata=TextParser.find(r"is a \w+ (\w+)"))
+        name: str = field(metadata=TextParser.find(r"^(\w+)"))
+        colour: Colour = field(metadata=Colour.make_parser())
+        age: int = field(metadata=TextParser.find_int(r"aged (\d+)"))
+
+    steph = Animal.parse("Stephanie is a white cat aged 10")
+    print(steph) # Animal(kind='cat', name='Stephanie', colour=<Colour.WHITE: 2>, age=10)
+"""
+
+import re
+from abc import ABC
+from dataclasses import MISSING, dataclass, fields
+from functools import partial
+from typing import Any, Callable, TypedDict, cast
+
+from typing_extensions import Self
+
+
+class ParserFn(TypedDict):
+    """Parser function in a dict compatible with the :func:`dataclasses.field` metadata param."""
+
+    #:
+    TextParser_fn: Callable[[str], Any]
+
+
+@dataclass
+class TextParser(ABC):
+    """Helper abstract dataclass that parses a text according to the fields' rules.
+
+    This class provides a selection of parser functions and a function to compose generic functions
+    with parser functions. Parser functions are designed to be passed to the fields' metadata param
+    to enable parsing.
+    """
+
+    """============ BEGIN PARSER FUNCTIONS ============"""
+
+    @staticmethod
+    def compose(f: Callable, parser_fn: ParserFn) -> ParserFn:
+        """Makes a composite parser function.
+
+        The parser function is run and if a non-None value was returned, f is called with it.
+        Otherwise the function returns early with None.
+
+        Metadata modifier for :func:`dataclasses.field`.
+
+        Args:
+            f: the function to apply to the parser's result
+            parser_fn: the dictionary storing the parser function
+        """
+        g = parser_fn["TextParser_fn"]
+
+        def _composite_parser_fn(text: str) -> Any:
+            intermediate_value = g(text)
+            if intermediate_value is None:
+                return None
+            return f(intermediate_value)
+
+        return ParserFn(TextParser_fn=_composite_parser_fn)
+
+    @staticmethod
+    def find(
+        pattern: str | re.Pattern[str],
+        flags: re.RegexFlag = re.RegexFlag(0),
+        named: bool = False,
+    ) -> ParserFn:
+        """Makes a parser function that finds a regular expression match in the text.
+
+        If the pattern has capturing groups, it returns None if no match was found. If the pattern
+        has only one capturing group and a match was found, its value is returned. If the pattern
+        has no capturing groups then either True or False is returned if the pattern had a match or
+        not.
+
+        Metadata modifier for :func:`dataclasses.field`.
+
+        Args:
+            pattern: the regular expression pattern
+            flags: the regular expression flags. Not used if the given pattern is already compiled
+            named: if set to True only the named capture groups will be returned as a dictionary
+        """
+        if isinstance(pattern, str):
+            pattern = re.compile(pattern, flags)
+
+        def _find(text: str) -> Any:
+            m = pattern.search(text)
+            if m is None:
+                return None if pattern.groups > 0 else False
+
+            if pattern.groups == 0:
+                return True
+
+            if named:
+                return m.groupdict()
+
+            matches = m.groups()
+            if len(matches) == 1:
+                return matches[0]
+
+            return matches
+
+        return ParserFn(TextParser_fn=_find)
+
+    @classmethod
+    def find_int(
+        cls,
+        pattern: str | re.Pattern[str],
+        flags: re.RegexFlag = re.RegexFlag(0),
+        int_base: int = 0,
+    ) -> ParserFn:
+        """Makes a parser function that converts the match of :meth:`~find` to int.
+
+        This function is compatible only with a pattern containing one capturing group.
+
+        Metadata modifier for :func:`dataclasses.field`.
+
+        Args:
+            pattern: the regular expression pattern
+            flags: the regular expression flags
+            int_base: the base of the number to convert from
+        Raises:
+            RuntimeError: if the pattern does not have exactly one capturing group
+        """
+        if isinstance(pattern, str):
+            pattern = re.compile(pattern, flags)
+
+        if pattern.groups != 1:
+            raise RuntimeError("only one capturing group is allowed with this parser function")
+
+        return cls.compose(partial(int, base=int_base), cls.find(pattern))
+
+    """============ END PARSER FUNCTIONS ============"""
+
+    @classmethod
+    def parse(cls, text: str) -> Self:
+        """Creates a new instance of the class from the given text.
+
+        A new class instance is created with all the fields that have a parser function in their
+        metadata. Fields without one are ignored and are expected to have a default value, otherwise
+        the class initialization will fail.
+
+        A field is populated with the value returned by its corresponding parser function.
+
+        Args:
+            text: the text to parse
+        Raises:
+            RuntimeError: if the parser did not find a match and the field does not have a default
+                          value or default factory.
+        """
+        fields_values = {}
+        for field in fields(cls):
+            parse = cast(ParserFn, field.metadata).get("TextParser_fn")
+            if parse is None:
+                continue
+
+            value = parse(text)
+            if value is not None:
+                fields_values[field.name] = value
+            elif field.default is MISSING and field.default_factory is MISSING:
+                raise RuntimeError(
+                    f"parser for field {field.name} returned None, but the field has no default"
+                )
+
+        return cls(**fields_values)
-- 
2.34.1

Re: [PATCH v2 3/5] dts: add parsing utility module

[Jeremy Spewock]

On Thu, May 9, 2024 at 7:26 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> Adds parsing text into a custom dataclass. It provides a new
> `TextParser` dataclass to be inherited. This implements the `parse`
> method, which combined with the parser functions, it can automatically
> parse the value for each field.
>
> This new utility will facilitate and simplify the parsing of complex
> command outputs, while ensuring that the codebase does not get bloated
> and stays flexible.
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> ---
Reviewed-by: Jeremy Spewock <jspewock@iol.unh.edu>

[PATCH v2 4/5] dts: add `show port info` command to TestPmdShell

[Luca Vizzarro]

Add a new TestPmdPort data structure to represent the output
returned by `show port info`, which is implemented as part of
TestPmdShell.

The TestPmdPort data structure and its derived classes are modelled
based on the relevant testpmd source code.

This implementation makes extensive use of regular expressions, which
all parse individually. The rationale behind this is to lower the risk
of the testpmd output changing as part of development. Therefore
minimising breakage.

Bugzilla ID: 1407

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
---
 dts/framework/remote_session/testpmd_shell.py | 490 +++++++++++++++++-
 1 file changed, 489 insertions(+), 1 deletion(-)

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index cb2ab6bd00..7910e17fed 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: BSD-3-Clause
 # Copyright(c) 2023 University of New Hampshire
 # Copyright(c) 2023 PANTHEON.tech s.r.o.
+# Copyright(c) 2024 Arm Limited
 
 """Testpmd interactive shell.
 
@@ -15,12 +16,17 @@
     testpmd_shell.close()
 """
 
+import re
 import time
-from enum import auto
+from dataclasses import dataclass, field
+from enum import Flag, auto
 from pathlib import PurePath
 from typing import Callable, ClassVar
 
+from typing_extensions import Self
+
 from framework.exception import InteractiveCommandExecutionError
+from framework.parser import ParserFn, TextParser
 from framework.settings import SETTINGS
 from framework.utils import StrEnum
 
@@ -80,6 +86,451 @@ class TestPmdForwardingModes(StrEnum):
     recycle_mbufs = auto()
 
 
+class VLANOffloadFlag(Flag):
+    """Flag representing the VLAN offload settings of a NIC port."""
+
+    #:
+    STRIP = auto()
+    #:
+    FILTER = auto()
+    #:
+    EXTEND = auto()
+    #:
+    QINQ_STRIP = auto()
+
+    @classmethod
+    def from_str_dict(cls, d):
+        """Makes an instance from a dict containing the flag member names with an "on" value."""
+        flag = cls(0)
+        for name in cls.__members__:
+            if d.get(name) == "on":
+                flag |= cls[name]
+        return flag
+
+    @classmethod
+    def make_parser(cls) -> ParserFn:
+        """Makes a parser function."""
+        return TextParser.compose(
+            cls.from_str_dict,
+            TextParser.find(
+                r"VLAN offload:\s+"
+                r"strip (?P<STRIP>on|off), "
+                r"filter (?P<FILTER>on|off), "
+                r"extend (?P<EXTEND>on|off), "
+                r"qinq strip (?P<QINQ_STRIP>on|off)$",
+                re.MULTILINE,
+                named=True,
+            ),
+        )
+
+
+class RSSOffloadTypesFlag(Flag):
+    """Flag representing the RSS offload flow types supported by the NIC port."""
+
+    #:
+    ipv4 = auto()
+    #:
+    ipv4_frag = auto()
+    #:
+    ipv4_tcp = auto()
+    #:
+    ipv4_udp = auto()
+    #:
+    ipv4_sctp = auto()
+    #:
+    ipv4_other = auto()
+    #:
+    ipv6 = auto()
+    #:
+    ipv6_frag = auto()
+    #:
+    ipv6_tcp = auto()
+    #:
+    ipv6_udp = auto()
+    #:
+    ipv6_sctp = auto()
+    #:
+    ipv6_other = auto()
+    #:
+    l2_payload = auto()
+    #:
+    ipv6_ex = auto()
+    #:
+    ipv6_tcp_ex = auto()
+    #:
+    ipv6_udp_ex = auto()
+    #:
+    port = auto()
+    #:
+    vxlan = auto()
+    #:
+    geneve = auto()
+    #:
+    nvgre = auto()
+    #:
+    user_defined_22 = auto()
+    #:
+    gtpu = auto()
+    #:
+    eth = auto()
+    #:
+    s_vlan = auto()
+    #:
+    c_vlan = auto()
+    #:
+    esp = auto()
+    #:
+    ah = auto()
+    #:
+    l2tpv3 = auto()
+    #:
+    pfcp = auto()
+    #:
+    pppoe = auto()
+    #:
+    ecpri = auto()
+    #:
+    mpls = auto()
+    #:
+    ipv4_chksum = auto()
+    #:
+    l4_chksum = auto()
+    #:
+    l2tpv2 = auto()
+    #:
+    ipv6_flow_label = auto()
+    #:
+    user_defined_38 = auto()
+    #:
+    user_defined_39 = auto()
+    #:
+    user_defined_40 = auto()
+    #:
+    user_defined_41 = auto()
+    #:
+    user_defined_42 = auto()
+    #:
+    user_defined_43 = auto()
+    #:
+    user_defined_44 = auto()
+    #:
+    user_defined_45 = auto()
+    #:
+    user_defined_46 = auto()
+    #:
+    user_defined_47 = auto()
+    #:
+    user_defined_48 = auto()
+    #:
+    user_defined_49 = auto()
+    #:
+    user_defined_50 = auto()
+    #:
+    user_defined_51 = auto()
+    #:
+    l3_pre96 = auto()
+    #:
+    l3_pre64 = auto()
+    #:
+    l3_pre56 = auto()
+    #:
+    l3_pre48 = auto()
+    #:
+    l3_pre40 = auto()
+    #:
+    l3_pre32 = auto()
+    #:
+    l2_dst_only = auto()
+    #:
+    l2_src_only = auto()
+    #:
+    l4_dst_only = auto()
+    #:
+    l4_src_only = auto()
+    #:
+    l3_dst_only = auto()
+    #:
+    l3_src_only = auto()
+
+    #:
+    ip = ipv4 | ipv4_frag | ipv4_other | ipv6 | ipv6_frag | ipv6_other | ipv6_ex
+    #:
+    udp = ipv4_udp | ipv6_udp | ipv6_udp_ex
+    #:
+    tcp = ipv4_tcp | ipv6_tcp | ipv6_tcp_ex
+    #:
+    sctp = ipv4_sctp | ipv6_sctp
+    #:
+    tunnel = vxlan | geneve | nvgre
+    #:
+    vlan = s_vlan | c_vlan
+    #:
+    all = (
+        eth
+        | vlan
+        | ip
+        | tcp
+        | udp
+        | sctp
+        | l2_payload
+        | l2tpv3
+        | esp
+        | ah
+        | pfcp
+        | gtpu
+        | ecpri
+        | mpls
+        | l2tpv2
+    )
+
+    @classmethod
+    def from_list_string(cls, names: str) -> Self:
+        """Makes a flag from a whitespace-separated list of names."""
+        flag = cls(0)
+        for name in names.split():
+            flag |= cls.from_str(name)
+        return flag
+
+    @classmethod
+    def from_str(cls, name: str) -> Self:
+        """Returns the flag corresponding to the supplied name."""
+        member_name = name.strip().replace("-", "_")
+        return cls[member_name]
+
+    def __str__(self):
+        """String representation."""
+        return self.name.replace("_", "-")
+
+    @classmethod
+    def make_parser(cls) -> ParserFn:
+        """Makes a parser function."""
+        return TextParser.compose(
+            RSSOffloadTypesFlag.from_list_string,
+            TextParser.find(r"Supported RSS offload flow types:((?:\r?\n?  \S+)+)", re.MULTILINE),
+        )
+
+
+class DeviceCapabilitiesFlag(Flag):
+    """Flag representing the device capabilities."""
+
+    RUNTIME_RX_QUEUE_SETUP = auto()
+    """Device supports Rx queue setup after device started."""
+    RUNTIME_TX_QUEUE_SETUP = auto()
+    """Device supports Tx queue setup after device started."""
+    RXQ_SHARE = auto()
+    """Device supports shared Rx queue among ports within Rx domain and switch domain."""
+    FLOW_RULE_KEEP = auto()
+    """Device supports keeping flow rules across restart."""
+    FLOW_SHARED_OBJECT_KEEP = auto()
+    """Device supports keeping shared flow objects across restart."""
+
+    @classmethod
+    def make_parser(cls) -> ParserFn:
+        """Makes a parser function."""
+        return TextParser.compose(
+            cls,
+            TextParser.find_int(r"Device capabilities: (0x[A-Fa-f\d]+)"),
+        )
+
+
+class DeviceErrorHandlingMode(StrEnum):
+    """Enum representing the device error handling mode."""
+
+    #:
+    none = auto()
+    #:
+    passive = auto()
+    #:
+    proactive = auto()
+    #:
+    unknown = auto()
+
+    @classmethod
+    def make_parser(cls) -> ParserFn:
+        """Makes a parser function."""
+        return TextParser.compose(cls, TextParser.find(r"Device error handling mode: (\w+)"))
+
+
+def make_device_private_info_parser() -> ParserFn:
+    """Device private information parser.
+
+    Ensure that we are not parsing invalid device private info output.
+    """
+
+    def _validate(info: str):
+        info = info.strip()
+        if info == "none" or info.startswith("Invalid file") or info.startswith("Failed to dump"):
+            return None
+        return info
+
+    return TextParser.compose(_validate, TextParser.find(r"Device private info:\s+([\s\S]+)"))
+
+
+@dataclass
+class TestPmdPort(TextParser):
+    """Dataclass representing the result of testpmd's ``show port info`` command."""
+
+    #:
+    id: int = field(metadata=TextParser.find_int(r"Infos for port (\d+)\b"))
+    #:
+    device_name: str = field(metadata=TextParser.find(r"Device name: ([^\r\n]+)"))
+    #:
+    driver_name: str = field(metadata=TextParser.find(r"Driver name: ([^\r\n]+)"))
+    #:
+    socket_id: int = field(metadata=TextParser.find_int(r"Connect to socket: (\d+)"))
+    #:
+    is_link_up: bool = field(metadata=TextParser.find("Link status: up"))
+    #:
+    link_speed: str = field(metadata=TextParser.find(r"Link speed: ([^\r\n]+)"))
+    #:
+    is_link_full_duplex: bool = field(metadata=TextParser.find("Link duplex: full-duplex"))
+    #:
+    is_link_autonegotiated: bool = field(metadata=TextParser.find("Autoneg status: On"))
+    #:
+    is_promiscuous_mode_enabled: bool = field(metadata=TextParser.find("Promiscuous mode: enabled"))
+    #:
+    is_allmulticast_mode_enabled: bool = field(
+        metadata=TextParser.find("Allmulticast mode: enabled")
+    )
+    #: Maximum number of MAC addresses
+    max_mac_addresses_num: int = field(
+        metadata=TextParser.find_int(r"Maximum number of MAC addresses: (\d+)")
+    )
+    #: Maximum configurable length of RX packet
+    max_hash_mac_addresses_num: int = field(
+        metadata=TextParser.find_int(r"Maximum number of MAC addresses of hash filtering: (\d+)")
+    )
+    #: Minimum size of RX buffer
+    min_rx_bufsize: int = field(metadata=TextParser.find_int(r"Minimum size of RX buffer: (\d+)"))
+    #: Maximum configurable length of RX packet
+    max_rx_packet_length: int = field(
+        metadata=TextParser.find_int(r"Maximum configurable length of RX packet: (\d+)")
+    )
+    #: Maximum configurable size of LRO aggregated packet
+    max_lro_packet_size: int = field(
+        metadata=TextParser.find_int(r"Maximum configurable size of LRO aggregated packet: (\d+)")
+    )
+
+    #: Current number of RX queues
+    rx_queues_num: int = field(metadata=TextParser.find_int(r"Current number of RX queues: (\d+)"))
+    #: Max possible RX queues
+    max_rx_queues_num: int = field(metadata=TextParser.find_int(r"Max possible RX queues: (\d+)"))
+    #: Max possible number of RXDs per queue
+    max_queue_rxd_num: int = field(
+        metadata=TextParser.find_int(r"Max possible number of RXDs per queue: (\d+)")
+    )
+    #: Min possible number of RXDs per queue
+    min_queue_rxd_num: int = field(
+        metadata=TextParser.find_int(r"Min possible number of RXDs per queue: (\d+)")
+    )
+    #: RXDs number alignment
+    rxd_alignment_num: int = field(metadata=TextParser.find_int(r"RXDs number alignment: (\d+)"))
+
+    #: Current number of TX queues
+    tx_queues_num: int = field(metadata=TextParser.find_int(r"Current number of TX queues: (\d+)"))
+    #: Max possible TX queues
+    max_tx_queues_num: int = field(metadata=TextParser.find_int(r"Max possible TX queues: (\d+)"))
+    #: Max possible number of TXDs per queue
+    max_queue_txd_num: int = field(
+        metadata=TextParser.find_int(r"Max possible number of TXDs per queue: (\d+)")
+    )
+    #: Min possible number of TXDs per queue
+    min_queue_txd_num: int = field(
+        metadata=TextParser.find_int(r"Min possible number of TXDs per queue: (\d+)")
+    )
+    #: TXDs number alignment
+    txd_alignment_num: int = field(metadata=TextParser.find_int(r"TXDs number alignment: (\d+)"))
+    #: Max segment number per packet
+    max_packet_segment_num: int = field(
+        metadata=TextParser.find_int(r"Max segment number per packet: (\d+)")
+    )
+    #: Max segment number per MTU/TSO
+    max_mtu_segment_num: int = field(
+        metadata=TextParser.find_int(r"Max segment number per MTU\/TSO: (\d+)")
+    )
+
+    #:
+    device_capabilities: DeviceCapabilitiesFlag = field(
+        metadata=DeviceCapabilitiesFlag.make_parser(),
+    )
+    #:
+    device_error_handling_mode: DeviceErrorHandlingMode = field(
+        metadata=DeviceErrorHandlingMode.make_parser()
+    )
+    #:
+    device_private_info: str | None = field(
+        default=None,
+        metadata=make_device_private_info_parser(),
+    )
+
+    #:
+    hash_key_size: int | None = field(
+        default=None, metadata=TextParser.find_int(r"Hash key size in bytes: (\d+)")
+    )
+    #:
+    redirection_table_size: int | None = field(
+        default=None, metadata=TextParser.find_int(r"Redirection table size: (\d+)")
+    )
+    #:
+    supported_rss_offload_flow_types: RSSOffloadTypesFlag = field(
+        default=RSSOffloadTypesFlag(0), metadata=RSSOffloadTypesFlag.make_parser()
+    )
+
+    #:
+    mac_address: str | None = field(
+        default=None, metadata=TextParser.find(r"MAC address: ([A-Fa-f0-9:]+)")
+    )
+    #:
+    fw_version: str | None = field(
+        default=None, metadata=TextParser.find(r"Firmware-version: ([^\r\n]+)")
+    )
+    #:
+    dev_args: str | None = field(default=None, metadata=TextParser.find(r"Devargs: ([^\r\n]+)"))
+    #: Socket id of the memory allocation
+    mem_alloc_socket_id: int | None = field(
+        default=None,
+        metadata=TextParser.find_int(r"memory allocation on the socket: (\d+)"),
+    )
+    #:
+    mtu: int | None = field(default=None, metadata=TextParser.find_int(r"MTU: (\d+)"))
+
+    #:
+    vlan_offload: VLANOffloadFlag | None = field(
+        default=None,
+        metadata=VLANOffloadFlag.make_parser(),
+    )
+
+    #: Maximum size of RX buffer
+    max_rx_bufsize: int | None = field(
+        default=None, metadata=TextParser.find_int(r"Maximum size of RX buffer: (\d+)")
+    )
+    #: Maximum number of VFs
+    max_vfs_num: int | None = field(
+        default=None, metadata=TextParser.find_int(r"Maximum number of VFs: (\d+)")
+    )
+    #: Maximum number of VMDq pools
+    max_vmdq_pools_num: int | None = field(
+        default=None, metadata=TextParser.find_int(r"Maximum number of VMDq pools: (\d+)")
+    )
+
+    #:
+    switch_name: str | None = field(
+        default=None, metadata=TextParser.find(r"Switch name: ([\r\n]+)")
+    )
+    #:
+    switch_domain_id: int | None = field(
+        default=None, metadata=TextParser.find_int(r"Switch domain Id: (\d+)")
+    )
+    #:
+    switch_port_id: int | None = field(
+        default=None, metadata=TextParser.find_int(r"Switch Port Id: (\d+)")
+    )
+    #:
+    switch_rx_domain: int | None = field(
+        default=None, metadata=TextParser.find_int(r"Switch Rx domain: (\d+)")
+    )
+
+
 class TestPmdShell(InteractiveShell):
     """Testpmd interactive shell.
 
@@ -225,6 +676,43 @@ def set_forward_mode(self, mode: TestPmdForwardingModes, verify: bool = True):
                 f"Test pmd failed to set fwd mode to {mode.value}"
             )
 
+    def show_port_info_all(self) -> list[TestPmdPort]:
+        """Returns the information of all the ports."""
+        output = self.send_command("show port info all")
+
+        # Sample output of the "all" command looks like:
+        #
+        # <start>
+        #
+        #   ********************* Infos for port 0 *********************
+        #   Key: value
+        #
+        #   ********************* Infos for port 1 *********************
+        #   Key: value
+        # <end>
+        #
+        # Take advantage of the double new line in between ports as end delimiter.
+        # But we need to artificially add a new line at the end to pick up the last port.
+        # Because commands are executed on a pseudo-terminal created by paramiko on the remote
+        # target lines end with CRLF. Therefore we also need to take the carriage return in account.
+        iter = re.finditer(r"\*{21}.*?[\r\n]{4}", output + "\r\n", re.S)
+        return [TestPmdPort.parse(block.group(0)) for block in iter]
+
+    def show_port_info(self, port_id: int) -> TestPmdPort:
+        """Returns the given port information.
+
+        Args:
+            port_id: The port ID to gather information for.
+
+        Raises:
+            InteractiveCommandExecutionError: If `port_id` is invalid.
+        """
+        output = self.send_command(f"show port info {port_id}", skip_first_line=True)
+        if output.startswith("Invalid port"):
+            raise InteractiveCommandExecutionError("invalid port given")
+
+        return TestPmdPort.parse(output)
+
     def close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
         self.send_command("quit", "")
-- 
2.34.1

Re: [PATCH v2 4/5] dts: add `show port info` command to TestPmdShell

[Jeremy Spewock]

All looks good to me, with one documentation nit-pick

On Thu, May 9, 2024 at 7:26 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> Add a new TestPmdPort data structure to represent the output
> returned by `show port info`, which is implemented as part of
> TestPmdShell.
>
> The TestPmdPort data structure and its derived classes are modelled
> based on the relevant testpmd source code.
>
> This implementation makes extensive use of regular expressions, which
> all parse individually. The rationale behind this is to lower the risk
> of the testpmd output changing as part of development. Therefore
> minimising breakage.
>
> Bugzilla ID: 1407
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> ---
<snip>
> +class DeviceCapabilitiesFlag(Flag):
> +    """Flag representing the device capabilities."""
> +
> +    RUNTIME_RX_QUEUE_SETUP = auto()
> +    """Device supports Rx queue setup after device started."""
> +    RUNTIME_TX_QUEUE_SETUP = auto()
> +    """Device supports Tx queue setup after device started."""
> +    RXQ_SHARE = auto()
> +    """Device supports shared Rx queue among ports within Rx domain and switch domain."""
> +    FLOW_RULE_KEEP = auto()
> +    """Device supports keeping flow rules across restart."""
> +    FLOW_SHARED_OBJECT_KEEP = auto()
> +    """Device supports keeping shared flow objects across restart."""
> +

In the other flag classes it looks like these doc-strings were changed
to be in the class var format (`#:`), but these might have been missed
when going through and changing the others.

> +    @classmethod
> +    def make_parser(cls) -> ParserFn:
<snip>
> 2.34.1
>

[PATCH v2 5/5] dts: add `show port stats` command to TestPmdShell

[Luca Vizzarro]

Add a new TestPmdPortStats data structure to represent the output
returned by `show port stats`, which is implemented as part of
TestPmdShell.

Bugzilla ID: 1407

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
---
 dts/framework/remote_session/testpmd_shell.py | 68 +++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index 7910e17fed..d0b6da50f0 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -531,6 +531,42 @@ class TestPmdPort(TextParser):
     )
 
 
+@dataclass
+class TestPmdPortStats(TextParser):
+    """Port statistics."""
+
+    #:
+    port_id: int = field(metadata=TextParser.find_int(r"NIC statistics for port (\d+)"))
+
+    #:
+    rx_packets: int = field(metadata=TextParser.find_int(r"RX-packets:\s+(\d+)"))
+    #:
+    rx_missed: int = field(metadata=TextParser.find_int(r"RX-missed:\s+(\d+)"))
+    #:
+    rx_bytes: int = field(metadata=TextParser.find_int(r"RX-bytes:\s+(\d+)"))
+    #:
+    rx_errors: int = field(metadata=TextParser.find_int(r"RX-errors:\s+(\d+)"))
+    #:
+    rx_nombuf: int = field(metadata=TextParser.find_int(r"RX-nombuf:\s+(\d+)"))
+
+    #:
+    tx_packets: int = field(metadata=TextParser.find_int(r"TX-packets:\s+(\d+)"))
+    #:
+    tx_errors: int = field(metadata=TextParser.find_int(r"TX-errors:\s+(\d+)"))
+    #:
+    tx_bytes: int = field(metadata=TextParser.find_int(r"TX-bytes:\s+(\d+)"))
+
+    #:
+    rx_pps: int = field(metadata=TextParser.find_int(r"Rx-pps:\s+(\d+)"))
+    #:
+    rx_bps: int = field(metadata=TextParser.find_int(r"Rx-bps:\s+(\d+)"))
+
+    #:
+    tx_pps: int = field(metadata=TextParser.find_int(r"Tx-pps:\s+(\d+)"))
+    #:
+    tx_bps: int = field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)"))
+
+
 class TestPmdShell(InteractiveShell):
     """Testpmd interactive shell.
 
@@ -713,6 +749,38 @@ def show_port_info(self, port_id: int) -> TestPmdPort:
 
         return TestPmdPort.parse(output)
 
+    def show_port_stats_all(self) -> list[TestPmdPortStats]:
+        """Returns the statistics of all the ports."""
+        output = self.send_command("show port stats all")
+
+        # Sample output of the "all" command looks like:
+        #
+        #   ########### NIC statistics for port 0 ###########
+        #   values...
+        #   #################################################
+        #
+        #   ########### NIC statistics for port 1 ###########
+        #   values...
+        #   #################################################
+        #
+        iter = re.finditer(r"(^  #*.+#*$[^#]+)^  #*$", output, re.MULTILINE)
+        return [TestPmdPortStats.parse(block.group(1)) for block in iter]
+
+    def show_port_stats(self, port_id: int) -> TestPmdPortStats:
+        """Returns the given port statistics.
+
+        Args:
+            port_id: The port ID to gather information for.
+
+        Raises:
+            InteractiveCommandExecutionError: If `port_id` is invalid.
+        """
+        output = self.send_command(f"show port stats {port_id}", skip_first_line=True)
+        if output.startswith("Invalid port"):
+            raise InteractiveCommandExecutionError("invalid port given")
+
+        return TestPmdPortStats.parse(output)
+
     def close(self) -> None:
         """Overrides :meth:`~.interactive_shell.close`."""
         self.send_command("quit", "")
-- 
2.34.1

Re: [PATCH v2 5/5] dts: add `show port stats` command to TestPmdShell

[Nicholas Pratte]

I've been running some local tests using a mock test suite. Each
method you created generates output as expected, but
show_port_stats_all() is printing out an empty list; I'm not sure this
is intentional or not. I don't have much experience with regular
expressions to be able to discern what is going on in the code within
a reasonable amount of time, so I wanted to bring this to your
attention.

On Thu, May 9, 2024 at 7:27 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> Add a new TestPmdPortStats data structure to represent the output
> returned by `show port stats`, which is implemented as part of
> TestPmdShell.
>
> Bugzilla ID: 1407
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> ---
>  dts/framework/remote_session/testpmd_shell.py | 68 +++++++++++++++++++
>  1 file changed, 68 insertions(+)
>
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index 7910e17fed..d0b6da50f0 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -531,6 +531,42 @@ class TestPmdPort(TextParser):
>      )
>
>
> +@dataclass
> +class TestPmdPortStats(TextParser):
> +    """Port statistics."""
> +
> +    #:
> +    port_id: int = field(metadata=TextParser.find_int(r"NIC statistics for port (\d+)"))
> +
> +    #:
> +    rx_packets: int = field(metadata=TextParser.find_int(r"RX-packets:\s+(\d+)"))
> +    #:
> +    rx_missed: int = field(metadata=TextParser.find_int(r"RX-missed:\s+(\d+)"))
> +    #:
> +    rx_bytes: int = field(metadata=TextParser.find_int(r"RX-bytes:\s+(\d+)"))
> +    #:
> +    rx_errors: int = field(metadata=TextParser.find_int(r"RX-errors:\s+(\d+)"))
> +    #:
> +    rx_nombuf: int = field(metadata=TextParser.find_int(r"RX-nombuf:\s+(\d+)"))
> +
> +    #:
> +    tx_packets: int = field(metadata=TextParser.find_int(r"TX-packets:\s+(\d+)"))
> +    #:
> +    tx_errors: int = field(metadata=TextParser.find_int(r"TX-errors:\s+(\d+)"))
> +    #:
> +    tx_bytes: int = field(metadata=TextParser.find_int(r"TX-bytes:\s+(\d+)"))
> +
> +    #:
> +    rx_pps: int = field(metadata=TextParser.find_int(r"Rx-pps:\s+(\d+)"))
> +    #:
> +    rx_bps: int = field(metadata=TextParser.find_int(r"Rx-bps:\s+(\d+)"))
> +
> +    #:
> +    tx_pps: int = field(metadata=TextParser.find_int(r"Tx-pps:\s+(\d+)"))
> +    #:
> +    tx_bps: int = field(metadata=TextParser.find_int(r"Tx-bps:\s+(\d+)"))
> +
> +
>  class TestPmdShell(InteractiveShell):
>      """Testpmd interactive shell.
>
> @@ -713,6 +749,38 @@ def show_port_info(self, port_id: int) -> TestPmdPort:
>
>          return TestPmdPort.parse(output)
>
> +    def show_port_stats_all(self) -> list[TestPmdPortStats]:
> +        """Returns the statistics of all the ports."""
> +        output = self.send_command("show port stats all")
> +
> +        # Sample output of the "all" command looks like:
> +        #
> +        #   ########### NIC statistics for port 0 ###########
> +        #   values...
> +        #   #################################################
> +        #
> +        #   ########### NIC statistics for port 1 ###########
> +        #   values...
> +        #   #################################################
> +        #
> +        iter = re.finditer(r"(^  #*.+#*$[^#]+)^  #*$", output, re.MULTILINE)
> +        return [TestPmdPortStats.parse(block.group(1)) for block in iter]
> +
> +    def show_port_stats(self, port_id: int) -> TestPmdPortStats:
> +        """Returns the given port statistics.
> +
> +        Args:
> +            port_id: The port ID to gather information for.
> +
> +        Raises:
> +            InteractiveCommandExecutionError: If `port_id` is invalid.
> +        """
> +        output = self.send_command(f"show port stats {port_id}", skip_first_line=True)
> +        if output.startswith("Invalid port"):
> +            raise InteractiveCommandExecutionError("invalid port given")
> +
> +        return TestPmdPortStats.parse(output)
> +
>      def close(self) -> None:
>          """Overrides :meth:`~.interactive_shell.close`."""
>          self.send_command("quit", "")
> --
> 2.34.1
>

Re: [PATCH v2 5/5] dts: add `show port stats` command to TestPmdShell

[Luca Vizzarro]

On 20/05/2024 15:26, Nicholas Pratte wrote:
> I've been running some local tests using a mock test suite. Each
> method you created generates output as expected, but
> show_port_stats_all() is printing out an empty list; I'm not sure this
> is intentional or not. I don't have much experience with regular
> expressions to be able to discern what is going on in the code within
> a reasonable amount of time, so I wanted to bring this to your
> attention.

Yeah, well spotted! I must have missed it in my testing... it looks like 
it's the fault of the sneaky \r being added at the end of the lines by 
the paramiko shell.

Re: [PATCH v2 5/5] dts: add `show port stats` command to TestPmdShell

[Jeremy Spewock]

Other than Nick's comment:

Reviewed-by: Jeremy Spewock <jspewock@iol.unh.edu>