Re: [PATCH] net/failsafe: fix parameters parsing

Hi Gaetan

-----Original Message-----
From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
Sent: Friday, August 18, 2017 12:03 PM
To: Matan Azrad <redacted>
Cc: dev@dpdk.org; Ori Kam <redacted>; stable@dpdk.org
Subject: Re: [PATCH] net/failsafe: fix parameters parsing

On Thu, Aug 17, 2017 at 06:52:48PM +0000, Matan Azrad wrote:

quoted

Hi Gaetan

quoted

-----Original Message-----
From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
Sent: Thursday, August 17, 2017 7:25 PM
To: Matan Azrad <redacted>
Cc: dev@dpdk.org; Ori Kam <redacted>; stable@dpdk.org
Subject: Re: [PATCH] net/failsafe: fix parameters parsing

On Thu, Aug 17, 2017 at 03:54:23PM +0000, Matan Azrad wrote:

quoted

quoted

-----Original Message-----
From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
Sent: Thursday, August 17, 2017 6:26 PM
To: Matan Azrad <redacted>
Cc: dev@dpdk.org; Ori Kam <redacted>;

stable@dpdk.org

quoted

quoted

quoted

quoted

Subject: Re: [PATCH] net/failsafe: fix parameters parsing

Hi Matan,

Thanks for spotting the bug.

On Thu, Aug 17, 2017 at 05:19:41PM +0300, Matan Azrad wrote:

quoted

The corrupted code used wrongly snprintf return value as the
number of characters actually copied, in spite of the meanning
is the number of characters which would be generated for the
given

input.

quoted

quoted

quoted

It caused to remain zerod bytes between the failsafe command
line non sub device parameters indicates end of string.

Hence, when rte_kvargs_parse tried to parse all parameters, it
got end of string after the first one and the others weren't parsed.

So, if the mac parameters was the first in command line it was
taken while hotplug_poll was left default, and vice versa.

The fix updates the buffer index by dedicated variable
contains the copy size, by the way uses memcpy instead of
snprintf which is good enouth for this copy scenario.

Actually snprintf should still be used.

Why?
We just want to copy from buffer to buffer, no needs snprintf
overheads If actually we are not using complicated format.

snprintf plays nice with strings and ensure that it will be terminated by a

'\0'.

quoted

quoted

It is generally preferable, particularly considering that the
parameter string we are building here is meant to be parsed by another

lib (rte_kvargs).

quoted

quoted

In this specific scenario it is not needed because we for sure know that the

length doesn't pass after '\0'.

quoted

By using memcpy we can prevent format parsing and other checks done by

snprintf.

quoted

This overhead is negligible.
snprintf is preferred over memcpy / strncpy in DPDK.

I don't think it is negligible (think only about the switch statement 
which should be used for format parsing).
In this case we know by sure that the copy size is not dangerous and
there is not justify to use snprintf function.

I think also if we wouldn't know the '\0' place,  it is preferred to use
strlen and memcpy instead of snprintf in any uncomplicated format cases.

Maybe it is behind to this fix(only enhancement) but I suggest you
to replace all failsafe snprintf usages for simple formats include this fix line
after this patch.

quoted

quoted

quoted

quoted

quoted

Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")
Cc: stable@dpdk.org

Signed-off-by: Matan Azrad <redacted>
---
 drivers/net/failsafe/failsafe_args.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/failsafe/failsafe_args.c

b/drivers/net/failsafe/failsafe_args.c
index 1f22416..0a98b04 100644

--- a/drivers/net/failsafe/failsafe_args.c
+++ b/drivers/net/failsafe/failsafe_args.c

@@ -271,7 +271,7 @@ static int

fs_remove_sub_devices_definition(char

params[DEVARGS_MAXLEN])

quoted

quoted

{

quoted

quoted

quoted

 	char buffer[DEVARGS_MAXLEN] = {0};
-	size_t a, b;
+	size_t a, b, temp;

temp is not descriptive enough.

What are about a, b, i ?

quoted

You are declaring this variable here because you want to re-use
it instead of `start`. This is an overreach however, this fix
must be restricted to the actual bug.

temp is helping to address the original bug, don't we want to
reuse variable Instead of 2  if statement variables, maybe other name

for all?

quoted

quoted

quoted

The compiler should be able to see that their use does not overlap.

This is new for me, thanks for the lesson.
I will update it to len & start as you suggested.

quoted

The issue is that temp is meant to describe a length limit in one
branch and be a marker of a starting point in another. Thus both
variable should be named differently to make the intent clear.

When declaring temp (or saved_val) in the higher-scope, you are thus
forced to use generic, non-descriptive name precisely because you
are sharing the variable between two different uses.

quoted

Something like:
a=> start
b=> end
i => next
temp=> saved_val

You didn't answer what are about my suggestions for new names instead
of a,b and I which are not descriptive too (like temp :))

Sure :) . Had I reviewed myself I would probably have asked for better
names.
I was writing the parsing utilities all at once and used the same conventions
throughout failsafe_args.c

There might be a patch at some point changing all those names.
But I think this is going beyond the scope of fixing the isolation of kvargs. My
point there is only to avoid making things worse than they are.

By sure this patch is not making  the things worse than they are(even v1) :)

quoted

quoted

quoted

quoted

quoted

 	int i;

 	a = 0;

@@ -286,12 +286,14 @@ fs_remove_sub_devices_definition(char

params[DEVARGS_MAXLEN])

quoted

 			ERROR("Invalid parameter");
 			return -EINVAL;
 		}
-		if (params[b] == ',' || params[b] == '\0')

Declare the temporary variable in this scope, with a descriptive
name such as "len", "length", "param_len" or something close.

quoted

-			i += snprintf(&buffer[i], b - a + 1, "%s",

&params[a]);

quoted

quoted

quoted

quoted

quoted

-		if (params[b] == '(') {
-			size_t start = b;
+		if (params[b] == ',' || params[b] == '\0') {
+			temp = b - a + 1;

The value here should be "b - a".
If a != 0 however, then we are parsing a new parameter and
buffer already contains at least one. A comma should be added to
separate the

two.

quoted

quoted

An example might clarify what I mean:

                if (params[b] == ',' || params[b] == '\0') {
                        size_t param_len = b - a;

                        if (a)
                                param_len += 1;
                        snprintf(&buffer[i], param_len + 1, "%s%s",
                                 a ? "," : "", &params[a]);
                        i += param_len;
                }

The conditionals about `a` are ugly however, if you find a
better way to write those you are most welcome :).

quoted

+			memcpy(&buffer[i], &params[a], temp);
+			i += temp;
+		} else if (params[b] == '(') {
+			temp = b;
 			b += closing_paren(&params[b]);
-			if (b == start)

I think the last comma is harmless for next parse But I can just
change the last coma to '\0' in the end of function(if exists).
But, This solves another issue, don't it?  Maybe in different patch?

It might be harmless, but I prefer having a clean output.
Which other issue? Handling the commas seems in line with properly
copying regular parameters.

Yes, but it is another issue in this specific copy (don't refer to
snprintf return value) But, it's OK, I think them both can be handle
by this patch :)

quoted

You could certainly check for the presence of a comma, but you
already see how ugly the code flow would be ("if it exists"), and
you will keep a dirty string between two loops, with possibly other
branches wanting to return on error.

I prefer having a compact, self-contained logic addressing kvargs
which should not leak conceptual gotcha that the next programmer
would need to be wary of when trying to touch the code in other parts of

the function.

quoted

quoted

If we leave always b-a+1 we potentially can get the next buffer strings:
p1,p2...pN,
or
p1,p2,pN(\0)
 while i is the index of the character after a comma or '\0'

so, for prevent the last comma we can override always in the end of

function(not between 2 loops) '\0' like this:

quoted

if (i >0 )
/* For prevent last comma. */
	buffer[i-1] = '\0';
instead of using 2 conditions per non sub device param - 'if(a)' and 'a ? "," :

""'

quoted

I think each future programmer can easily understand it.

Sure, I agree that it would work. It would also probably be parseable.
But it depends on your implementation. I think you'll have to write this
`buffer[i-1] = '\0';` outside the branch dealing with proper kvargs, which
mean scattering within the rest of the function code that should be dealing
with how those kvargs are presented to the next stage.

I think the most constructive thing to do would be for you to write a v2
proposing your solution. If you are able to contain all logic pertaining to the
kvargs within this single branch along with the other remarks (keeping
snprintf, using a local "len" variable), then perfect :). Otherwise, while
imperfect I proposed an alternative that should be able to do that.

OK, I will create V2, thanks for this review!

quoted

quoted

quoted

quoted

The changes should be limited to the actual bug. No need to change

this.

quoted

quoted

quoted

quoted

quoted

+			if (b == temp)
 				return -EINVAL;
 			b += 1;
 			if (params[b] == '\0')
--
2.7.4

Thanks again for the debug and sorry for being nitpicky.

--
Gaëtan Rivet
6WIND

Regards,
Matan Azrad.

--
Gaëtan Rivet
6WIND

Regards
Matan Azrad.

--
Gaëtan Rivet
6WIND

Regards
Matan Azrad