diff mbox

[dpdk-dev] acl: remove invalid test

Message ID c0b4fbc916008d8c8e47707889eeac545a10eb0a.1481735746.git.mirq-linux@rere.qmqm.pl (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel compilation success Compilation OK

Commit Message

Michał Mirosław Dec. 14, 2016, 5:23 p.m. UTC 
  rte_acl_add_rules() has no way of checking rule size.

This was hidden because the test effectively checked that
adding a rule with userdata == 0 failed.

Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>
---
 app/test/test_acl.c | 20 --------------------
 1 file changed, 20 deletions(-)

Comments

Ananyev, Konstantin Dec. 19, 2016, 6:48 p.m. UTC | #1 
Hi Michal,

> -----Original Message-----

> From: Michał Mirosław [mailto:mirq-linux@rere.qmqm.pl]

> Sent: Wednesday, December 14, 2016 5:24 PM

> To: dev@dpdk.org

> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>

> Subject: [PATCH] acl: remove invalid test

> 

> rte_acl_add_rules() has no way of checking rule size.

> 

> This was hidden because the test effectively checked that

> adding a rule with userdata == 0 failed.


I suppose that changes have to be inside:
[PATCH v2] acl: allow zero verdict.

Konstantin 


> 

> Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>

> ---

>  app/test/test_acl.c | 20 --------------------

>  1 file changed, 20 deletions(-)

> 

> diff --git a/app/test/test_acl.c b/app/test/test_acl.c

> index 28955f0..be744ec 100644

> --- a/app/test/test_acl.c

> +++ b/app/test/test_acl.c

> @@ -1515,26 +1515,6 @@ test_invalid_parameters(void)

>  	/* free ACL context */

>  	rte_acl_free(acx);

> 

> -	/* set wrong rule_size so that adding any rules would fail */

> -	param.rule_size = RTE_ACL_IPV4VLAN_RULE_SZ + 4;

> -	acx = rte_acl_create(&param);

> -	if (acx == NULL) {

> -		printf("Line %i: ACL context creation failed!\n", __LINE__);

> -		return -1;

> -	}

> -

> -	/* try adding a rule with size different from context rule_size */

> -	result = rte_acl_ipv4vlan_add_rules(acx, &rule, 1);

> -	if (result == 0) {

> -		printf("Line %i: Adding an invalid sized rule "

> -				"should have failed!\n", __LINE__);

> -		rte_acl_free(acx);

> -		return -1;

> -	}

> -

> -	/* free ACL context */

> -	rte_acl_free(acx);

> -

> 

>  	/**

>  	 * rte_acl_ipv4vlan_build

> --

> 2.10.2
Michał Mirosław Dec. 23, 2016, 1:47 a.m. UTC | #2 
On Mon, Dec 19, 2016 at 06:48:52PM +0000, Ananyev, Konstantin wrote:
> Hi Michal,
> 
> > -----Original Message-----
> > From: Michał Mirosław [mailto:mirq-linux@rere.qmqm.pl]
> > Sent: Wednesday, December 14, 2016 5:24 PM
> > To: dev@dpdk.org
> > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > Subject: [PATCH] acl: remove invalid test
> > 
> > rte_acl_add_rules() has no way of checking rule size.
> > 
> > This was hidden because the test effectively checked that
> > adding a rule with userdata == 0 failed.
> I suppose that changes have to be inside:
> [PATCH v2] acl: allow zero verdict.

The 'allow zero verdict' patch depends on this one if we are to not have
a breaking tests inbetween. Otherwise, it is an independent change.

I guess I can merge them, though, if you prefer it that way.

Best Regards,
Michał Mirosław
Ananyev, Konstantin Dec. 23, 2016, 9:36 a.m. UTC | #3 
> -----Original Message-----
> From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl]
> Sent: Friday, December 23, 2016 1:48 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH] acl: remove invalid test
> 
> On Mon, Dec 19, 2016 at 06:48:52PM +0000, Ananyev, Konstantin wrote:
> > Hi Michal,
> >
> > > -----Original Message-----
> > > From: Michał Mirosław [mailto:mirq-linux@rere.qmqm.pl]
> > > Sent: Wednesday, December 14, 2016 5:24 PM
> > > To: dev@dpdk.org
> > > Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > > Subject: [PATCH] acl: remove invalid test
> > >
> > > rte_acl_add_rules() has no way of checking rule size.
> > >
> > > This was hidden because the test effectively checked that
> > > adding a rule with userdata == 0 failed.
> > I suppose that changes have to be inside:
> > [PATCH v2] acl: allow zero verdict.
> 
> The 'allow zero verdict' patch depends on this one if we are to not have
> a breaking tests inbetween. 

Exactly, that's why I think they either has to be in one series of patches,
with this one coming first and ' PATCH v2] acl: allow zero verdict' as the second one,
or just merge them into one.

Konstantin

>Otherwise, it is an independent change.
> 
> I guess I can merge them, though, if you prefer it that way.
> 
> Best Regards,
> Michał Mirosław
Thomas Monjalon Jan. 17, 2017, 3:24 p.m. UTC | #4 
2016-12-23 09:36, Ananyev, Konstantin:
> From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl]
> > On Mon, Dec 19, 2016 at 06:48:52PM +0000, Ananyev, Konstantin wrote:
> > > I suppose that changes have to be inside:
> > > [PATCH v2] acl: allow zero verdict.
> > 
> > The 'allow zero verdict' patch depends on this one if we are to not have
> > a breaking tests inbetween. 
> 
> Exactly, that's why I think they either has to be in one series of patches,
> with this one coming first and ' PATCH v2] acl: allow zero verdict' as the second one,
> or just merge them into one.

No progress here.
Konstantin, do you ack this patch?
I could apply it as a standalone patch.
Ananyev, Konstantin Jan. 18, 2017, 9:51 a.m. UTC | #5 
Hi Thomas,

> 
> 2016-12-23 09:36, Ananyev, Konstantin:
> > From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl]
> > > On Mon, Dec 19, 2016 at 06:48:52PM +0000, Ananyev, Konstantin wrote:
> > > > I suppose that changes have to be inside:
> > > > [PATCH v2] acl: allow zero verdict.
> > >
> > > The 'allow zero verdict' patch depends on this one if we are to not have
> > > a breaking tests inbetween.
> >
> > Exactly, that's why I think they either has to be in one series of patches,
> > with this one coming first and ' PATCH v2] acl: allow zero verdict' as the second one,
> > or just merge them into one.
> 
> No progress here.
> Konstantin, do you ack this patch?

Yes, I do.
I just thought that the author would resubmit it as part of 
' PATCH v2] acl: allow zero verdict'
to comply with DPDK patch submission rules.
Konstantin

> I could apply it as a standalone patch.
Michał Mirosław Jan. 18, 2017, 7:21 p.m. UTC | #6 
On Wed, Jan 18, 2017 at 09:51:45AM +0000, Ananyev, Konstantin wrote:
> Hi Thomas,
> 
> > 
> > 2016-12-23 09:36, Ananyev, Konstantin:
> > > From: Michal Miroslaw [mailto:mirq-linux@rere.qmqm.pl]
> > > > On Mon, Dec 19, 2016 at 06:48:52PM +0000, Ananyev, Konstantin wrote:
> > > > > I suppose that changes have to be inside:
> > > > > [PATCH v2] acl: allow zero verdict.
> > > >
> > > > The 'allow zero verdict' patch depends on this one if we are to not have
> > > > a breaking tests inbetween.
> > >
> > > Exactly, that's why I think they either has to be in one series of patches,
> > > with this one coming first and ' PATCH v2] acl: allow zero verdict' as the second one,
> > > or just merge them into one.
> > No progress here.
> > Konstantin, do you ack this patch?
> Yes, I do.
> I just thought that the author would resubmit it as part of 
> ' PATCH v2] acl: allow zero verdict'
> to comply with DPDK patch submission rules.
> Konstantin
> > I could apply it as a standalone patch.

Sorry for the delay. I'll do just that in a moment.

Best Regards,
Michał Mirosław
diff mbox

Patch

diff --git a/app/test/test_acl.c b/app/test/test_acl.c
index 28955f0..be744ec 100644
--- a/app/test/test_acl.c
+++ b/app/test/test_acl.c
@@ -1515,26 +1515,6 @@  test_invalid_parameters(void)
 	/* free ACL context */
 	rte_acl_free(acx);
 
-	/* set wrong rule_size so that adding any rules would fail */
-	param.rule_size = RTE_ACL_IPV4VLAN_RULE_SZ + 4;
-	acx = rte_acl_create(&param);
-	if (acx == NULL) {
-		printf("Line %i: ACL context creation failed!\n", __LINE__);
-		return -1;
-	}
-
-	/* try adding a rule with size different from context rule_size */
-	result = rte_acl_ipv4vlan_add_rules(acx, &rule, 1);
-	if (result == 0) {
-		printf("Line %i: Adding an invalid sized rule "
-				"should have failed!\n", __LINE__);
-		rte_acl_free(acx);
-		return -1;
-	}
-
-	/* free ACL context */
-	rte_acl_free(acx);
-
 
 	/**
 	 * rte_acl_ipv4vlan_build