Re: [PATCH] app/testpmd: fix auto completion for indirect list action

Re: [PATCH] app/testpmd: fix auto completion for indirect list action

[Ferruh Yigit]

On 3/19/2024 2:51 PM, Ferruh Yigit wrote:
> On 3/18/2024 9:21 AM, Shani Peretz wrote:
>> In the process of auto completion of a command in testpmd,
>> the parser splits the command into tokens, where each token
>> represents an argument and defines a parsing function.
>> The parsing function of the indirect_list action argument was returning
>> before having the opportunity to handle the argument.
>>
> Hi Shani,
> 
> I can see a few other handles follows the updated logic, but to
> understand more, was the problematic part following:
> ```
> 	if (!action)
> 		return -1;
> ```
> 
> If so why 'action' can be NULL and why need to continue for this case,
> can you please help me understand?
> 
> Also even if 'action' is NULL, function will return output of
> 'parse_int()', is this expected?
> 

I can verify the fix via debugging,

it seems missing ".comp = comp_none" cause calling handler
(parse_indlst_id2ptr), and 'parse_indlst_id2ptr()' needs to be fixed to
parse correctly.

I will proceed with patch since it is local to a specific flow command,


BUT overall how can we catch issues like this in the feature, we don't
have a good way to test testpmd flow commands.
@Ori, @Gregory, do you have any idea?
cc'ed CI mail list too.

Re: [PATCH] app/testpmd: fix auto completion for indirect list action

[Gregory Etelson]

[-- Attachment #1: Type: text/plain, Size: 711 bytes --]

Hello Ferruh,

>BUT overall how can we catch issues like this in the feature, we don't
>have a good way to test testpmd flow commands.
>@Ori, @Gregory, do you have any idea?
>cc'ed CI mail list too.

We have a tool for unit tests based on the testpmd.
The tool details are here:  https://drive.google.com/drive/folders/1cHrPwx4fUJ6ibUCtHd4kNKsrmmvQvvOj?usp=drive_link.
There's also a short description here: https://inbox.dpdk.org/ci/2a287ee7-cda4-f2ab-a4e6-a47021f8573f@nvidia.com/

Consider an option when a code patch is accompanied with a short test script that validates that patch functionality.
DPDK CI can run the script to verify that the patch functions correctly.

Regards,
Gregory


[-- Attachment #2: Type: text/html, Size: 3787 bytes --]

Re: [PATCH] app/testpmd: fix auto completion for indirect list action

[Ferruh Yigit]

On 3/20/2024 6:06 AM, Gregory Etelson wrote:
> Hello Ferruh,
> 
>>BUT overall how can we catch issues like this in the feature, we don't
>>have a good way to test testpmd flow commands.
>>@Ori, @Gregory, do you have any idea?
>>cc'ed CI mail list too.
> 
> We have a tool for unit tests based on the testpmd.
> The tool details are here: 
> https://drive.google.com/drive/folders/1cHrPwx4fUJ6ibUCtHd4kNKsrmmvQvvOj?usp=drive_link <https://drive.google.com/drive/folders/1cHrPwx4fUJ6ibUCtHd4kNKsrmmvQvvOj?usp=drive_link>.
> There's also a short description here:
> https://inbox.dpdk.org/ci/2a287ee7-cda4-f2ab-a4e6-a47021f8573f@nvidia.com/ <https://inbox.dpdk.org/ci/2a287ee7-cda4-f2ab-a4e6-a47021f8573f@nvidia.com/>
> 
> Consider an option when a code patch is accompanied with a short test
> script that validates that patch functionality.
> DPDK CI can run the script to verify that the patch functions correctly.
> 
>

Thanks Gregory, I missed this proposal, we need something to verify flow
APIs, so +1 to the effort.
What is the status of incorporating this feature into dts?


But I guess it won't catch this issue, as it uses full flow commands.
This issue is related to the testpmd command parsing code. I wonder if
we can find a way to verify testpmd parsing code?

Re: [PATCH] app/testpmd: fix auto completion for indirect list action

[Patrick Robb]

On Wed, Mar 20, 2024 at 6:08 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 3/20/2024 6:06 AM, Gregory Etelson wrote:
> > Hello Ferruh,
> >
> >>BUT overall how can we catch issues like this in the feature, we don't
> >>have a good way to test testpmd flow commands.
> >>@Ori, @Gregory, do you have any idea?
> >>cc'ed CI mail list too.
> >
> > We have a tool for unit tests based on the testpmd.
> > The tool details are here:
> > https://drive.google.com/drive/folders/1cHrPwx4fUJ6ibUCtHd4kNKsrmmvQvvOj?usp=drive_link <https://drive.google.com/drive/folders/1cHrPwx4fUJ6ibUCtHd4kNKsrmmvQvvOj?usp=drive_link>.
> > There's also a short description here:
> > https://inbox.dpdk.org/ci/2a287ee7-cda4-f2ab-a4e6-a47021f8573f@nvidia.com/ <https://inbox.dpdk.org/ci/2a287ee7-cda4-f2ab-a4e6-a47021f8573f@nvidia.com/>
> >
> > Consider an option when a code patch is accompanied with a short test
> > script that validates that patch functionality.
> > DPDK CI can run the script to verify that the patch functions correctly.

A similar idea has been proposed for DTS. I believe the plan is at the
end of 2024 techboard is going to discuss adding this as a requirement
for patches affecting certain components in DPDK. So a change related
to the new feature must come with an accompanied /dts testsuite
addition at the same time.

> >
> >
>
> Thanks Gregory, I missed this proposal, we need something to verify flow
> APIs, so +1 to the effort.
> What is the status of incorporating this feature into dts?

The DTS 24.07 roadmap does not include testsuites verifying flow APIs,
but if work proceeds at a good pace and there is space for it, we can
look at writing something for 24.07. Otherwise it falls to 24.11 or
25.03. Sorry, I know that's pretty far out.

>
>
> But I guess it won't catch this issue, as it uses full flow commands.
> This issue is related to the testpmd command parsing code. I wonder if
> we can find a way to verify testpmd parsing code?
>

Re: [PATCH] app/testpmd: fix auto completion for indirect list action

[Gregory Etelson]

[-- Attachment #1: Type: text/plain, Size: 5884 bytes --]

Hello Ferruh,

> But I guess it won't catch this issue, as it uses full flow commands.
> This issue is related to the testpmd command parsing code. I wonder if
> we can find a way to verify testpmd parsing code?

What if flow command validation program breaks a tested command into tokens and then pass each token followed by the TAB key to the controlled testpmd process ?
There are 3 possible outputs for the token + TAB sequence:

  1.  a valid token will trigger meaningful testpmd help ouput that can be matched.
  2.  invalid token output will fail [1].
  3.
invalid token will crash the controlled testpmd process.

I've compiled a small programm to test that idea.
Please check it out.

Regards,
Gregory.

commit e4a27c2c2892cfd408b473b18192b30927e4281c
Author: Gregory Etelson <getelson@nvidia.com>
Date:   Sat Mar 23 11:38:44 2024 +0200

   validate testpmd flow command

diff --git a/Cargo.toml b/Cargo.toml
new file mode 100644
index 0000000..3498868
--- /dev/null
+++ b/Cargo.toml
@@ -0,0 +1,12 @@
+[package]
+name = "testpmd-validate"
+version = "0.1.0"
+edition = "2021"
+rust-version = "1.76.0"
+
+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+
+[dependencies]
+ssh2 = { version = "0.9.4", features = ["vendored-openssl"] }
+time = "0.3.34"
+regex = "1.10.4"
diff --git a/src/main.rs b/src/main.rs
new file mode 100644
index 0000000..90dfdac
--- /dev/null
+++ b/src/main.rs
@@ -0,0 +1,128 @@
+use std::io::{Read, Write};
+use std::{io, thread, time};
+use std::path::Path;
+use std::ops::Add;
+use std::time::{SystemTime};
+use ssh2::{Channel, Error, ExtendedData, Session};
+use regex::{RegexSet};
+
+fn continue_after_error(err:&io::Error) -> bool {
+    match err.kind() {
+        io::ErrorKind::WouldBlock => return true,
+        _ => return false,
+    }
+}
+
+fn wrap_ssh<F, T>(mut func:F) -> T
+    where F: FnMut() -> Result<T, Error> {
+    loop {
+        match func() {
+            Ok(val) => { return val },
+            Err(ssh_err) => {
+                let io_err = io::Error::from(ssh_err);
+                if !continue_after_error(&io_err) {
+                    panic!("ssh2 error: {:?}", io_err);
+                }
+            }
+        }
+    }
+}
+
+fn rsh_write(channel:&mut Channel, buf:&str) {
+    match channel.write(buf.as_bytes()) {
+        Ok(_) => (),
+        Err(e) => panic!("write failure: {:?}", e)
+    }
+}
+
+const RSH_READ_MAX_DURATION:time::Duration = time::Duration::from_millis(350);
+const RSH_READ_IDLE_TIMEOUT:time::Duration = time::Duration::from_millis(10);
+fn rsh_read(channel:&mut Channel) -> String  {
+    let mut end = SystemTime::now().add(RSH_READ_MAX_DURATION);
+    let mut output = String::new();
+    loop {
+        let mut buffer: [u8; 1024] = [0; 1024];
+        match channel.read(&mut buffer) {
+            Ok(0)  => { return output }
+            Ok(_) => {
+                let aux = String::from_utf8(buffer.into()).unwrap();
+                // println!("{aux}");
+                output.push_str(&aux);
+                end = SystemTime::now().add(RSH_READ_MAX_DURATION);
+            }
+            Err(err) => {
+                if !continue_after_error(&err) { panic!("read error {:?}", err) }
+                if SystemTime::now().gt(&end) { break; }
+                thread::sleep(RSH_READ_IDLE_TIMEOUT);
+            }
+        }
+    }
+    output
+}
+
+const TESTPMD_PATH:&str = "/tmp/build/app";
+const TESTPMD_CMD:&str = "dpdk-testpmd -a 0000:01:00.0 -- -i";
+
+const TOKEN_PATTERN:[&str; 2] = [
+    r#"(?m)\[.*\]:"#,
+    r#"(?m)\[RETURN]"#,
+];
+
+const CTRL_C:[u8;1] = [0x3];
+
+
+fn rsh_connect(hostname:&str) -> Channel {
+    let stream = std::net::TcpStream::connect(format!("{}:22", hostname)).unwrap();
+    stream.set_nonblocking(true).unwrap();
+    let mut session = Session::new().unwrap();
+    let private_key = Path::new("/home/garik/.ssh/id_rsa");
+    session.set_blocking(false);
+    session.set_tcp_stream(stream);
+    wrap_ssh(|| session.handshake());
+    wrap_ssh(|| session.userauth_pubkey_file("root", None, &private_key, None));
+    let mut ch = wrap_ssh(|| session.channel_session());
+    wrap_ssh(|| ch.handle_extended_data(ExtendedData::Merge));
+    wrap_ssh(|| ch.request_pty("vt100", None, None));
+    ch
+}
+
+fn validate_flow_command(command:&str, ch:&mut Channel) {
+    let rset = RegexSet::new(TOKEN_PATTERN).unwrap();
+    let delay = time::Duration::from_millis(300);
+
+    println!("validate: {command}");
+    for token in command.split_whitespace() {
+        let candid = format!("{token} \t");
+        rsh_write(ch, &candid);
+        print!("validate token \'{token}\' ");
+        thread::sleep(delay);
+        let output = rsh_read(ch);
+        if rset.is_match(&output) {
+            println!(" ok");
+        } else {
+            println!("failed");
+            println!(">>>>>>>>>>>>>>>>>>");
+            println!("{output}");
+            println!("<<<<<<<<<<<<<<<<<<");
+            break;
+        }
+    }
+    let _ = ch.write(&CTRL_C);
+}
+
+fn main() {
+    let mut ch = rsh_connect("10.237.157.85");
+    let command = format!("{}/{}", TESTPMD_PATH, TESTPMD_CMD);
+    wrap_ssh(|| ch.exec(&command));
+    thread::sleep(time::Duration::from_secs(1));
+    println!("{}", rsh_read(&mut ch));
+
+    let good_flow = "flow create 0 ingress group 0 priority 0 pattern eth / end actions drop / end";
+    let bad_flow = "flow create 0 ingress group 0 priority 0 pattern eth/ end actions drop / end";
+
+    validate_flow_command(good_flow, &mut ch);
+    validate_flow_command(bad_flow, &mut ch);
+
+    wrap_ssh(|| ch.close());
+    println!("EOF {:?}", ch.exit_status());
+}




[-- Attachment #2: Type: text/html, Size: 39583 bytes --]