app/testpmd: fix auto completion for indirect list action

  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.

The fix ensures that the function appropriately handles
the argument before finishing.

Fixes: 72a3dec7126f ("ethdev: add indirect flow list action")

Signed-off-by: Shani Peretz <shperetz@nvidia.com>
---
 app/test-pmd/cmdline_flow.c | 46 ++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 21 deletions(-)
  

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?


Thanks,
ferruh

> The fix ensures that the function appropriately handles
> the argument before finishing.
> 
> Fixes: 72a3dec7126f ("ethdev: add indirect flow list action")
> 
> Signed-off-by: Shani Peretz <shperetz@nvidia.com>
> ---
>  app/test-pmd/cmdline_flow.c | 46 ++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 21 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index fd6c51f72d..60ee9337cf 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -7839,11 +7839,13 @@ static const struct token token_list[] = {
>  		.type = "UNSIGNED",
>  		.help = "unsigned integer value",
>  		.call = parse_indlst_id2ptr,
> +		.comp = comp_none,
>  	},
>  	[INDIRECT_LIST_ACTION_ID2PTR_CONF] = {
>  		.type = "UNSIGNED",
>  		.help = "unsigned integer value",
>  		.call = parse_indlst_id2ptr,
> +		.comp = comp_none,
>  	},
>  	[ACTION_SHARED_INDIRECT] = {
>  		.name = "shared_indirect",
> @@ -11912,34 +11914,36 @@ parse_indlst_id2ptr(struct context *ctx, const struct token *token,
>  	uint32_t id;
>  	int ret;
>  
> -	if (!action)
> -		return -1;
>  	ctx->objdata = 0;
>  	ctx->object = &id;
>  	ctx->objmask = NULL;
>  	ret = parse_int(ctx, token, str, len, ctx->object, sizeof(id));
> +	ctx->object = action;
>  	if (ret != (int)len)
>  		return ret;
> -	ctx->object = action;
> -	action_conf = (void *)(uintptr_t)action->conf;
> -	action_conf->conf = NULL;
> -	switch (ctx->curr) {
> -	case INDIRECT_LIST_ACTION_ID2PTR_HANDLE:
> -	action_conf->handle = (typeof(action_conf->handle))
> -				port_action_handle_get_by_id(ctx->port, id);
> -		if (!action_conf->handle) {
> -			printf("no indirect list handle for id %u\n", id);
> -			return -1;
> +
> +	/* set handle and conf */
> +	if (action) {
> +		action_conf = (void *)(uintptr_t)action->conf;
> +		action_conf->conf = NULL;
> +		switch (ctx->curr) {
> +		case INDIRECT_LIST_ACTION_ID2PTR_HANDLE:
> +		action_conf->handle = (typeof(action_conf->handle))
> +					port_action_handle_get_by_id(ctx->port, id);
> +			if (!action_conf->handle) {
> +				printf("no indirect list handle for id %u\n", id);
> +				return -1;
> +			}
> +			break;
> +		case INDIRECT_LIST_ACTION_ID2PTR_CONF:
> +			indlst_conf = indirect_action_list_conf_get(id);
> +			if (!indlst_conf)
> +				return -1;
> +			action_conf->conf = (const void **)indlst_conf->conf;
> +			break;
> +		default:
> +			break;
>  		}
> -		break;
> -	case INDIRECT_LIST_ACTION_ID2PTR_CONF:
> -		indlst_conf = indirect_action_list_conf_get(id);
> -		if (!indlst_conf)
> -			return -1;
> -		action_conf->conf = (const void **)indlst_conf->conf;
> -		break;
> -	default:
> -		break;
>  	}
>  	return ret;
>  }
  

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.
  

On 3/19/2024 3:29 PM, Ferruh Yigit wrote:
> 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,
> 

Tested-by: Ferruh Yigit <ferruh.yigit@amd.com>

Applied to dpdk-next-net/main, thanks.
  

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
  

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?
  

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?
>
  

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());
+}