Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PATCH v1] test: validation: ipsec: add antireplay tests for larger window sizes #1264

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mrchalla
Copy link
Contributor

@mrchalla mrchalla commented Apr 28, 2021

A reference validation test for exercising more antireplay tests
required for validating the AR larger window sizes(PR #1261).

Signed-off-by: Mahipal Challa mchalla@marvell.com

A reference validation test for exercising more antireplay tests
required for validating the larger window sizes(PR#1261).

Signed-off-by: Mahipal Challa <mchalla@marvell.com>
@odpbuild odpbuild changed the title test: validation: ipsec: add antireplay tests for larger window sizes [PATCH v1] test: validation: ipsec: add antireplay tests for larger window sizes Apr 28, 2021
@@ -937,6 +937,73 @@ void ipsec_check_out_one(const ipsec_test_part *part, odp_ipsec_sa_t sa)
odp_packet_free(pkto[i]);
}

static void ipsec_pkt_proto_err_set(odp_packet_t pkt)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to have a separate patch for moving the existing code around. Then the actual change is not mixed with noise and it easier to read the commit later. That said, I am not sure this code should be moved here (as opposed to staying in where it is or moving to a new .c file).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to have a separate patch for moving the existing code around. Then the actual change is not mixed with noise and it easier to read the commit later.

Ack.

I am not sure this code should be moved here (as opposed to staying in where it is or moving to a new .c file).

As we are discussing the code organization in a separate email thread, once decision is made, will plan to make the changes as per that decision.


/* 1. Advance the TOP of the window to WS * 2 */
sa_param.seq_num = window_sz * 2;
CU_ASSERT_EQUAL(0, odp_ipsec_test_sa_update(out_sa,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of odp_ipsec_test_sa_update() is not very nice since it is an optional part of the API and may not be supported by all implementations. Failing a validation test because odp_ipsec_test_sa_update() fails is, strictly speaking, not correct, although with current implementations it would work.

I do not think the use of odp_ipsec_test_sa_update() is necessary here. The test case could just process enough packets using the outbound SA to get the sequence number where it wants it and then after that do the actual test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of odp_ipsec_test_sa_update() is not very nice since it is an optional part of the API and may not be supported by all implementations

We have just used this available API for updating the required seq number. Does "optional part of the API" mean, will it be deprecated soon.

I do not think the use of odp_ipsec_test_sa_update() is necessary here. The test case could just process enough packets using the outbound SA to get the sequence number where it wants it and then after that do the actual test.

Sure, we will explore this thought.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have just used this available API for updating the required seq number. Does "optional part of the API" mean, will it be deprecated soon.

Optionality means that an ODP implementation is not required to support the API. There is capability info for the sequence number update support and in addition to that the API says that the function is not required to always succeed.

Since this is API validation test code, it is not correct to fail the test (i.e. essentially declare the implementation under test as API-incompliant) just because an optional testing feature is not supported. At minimum the validation test should check the relevant capability and be prepared for the SA update function to fail. Since this would mean that the test case could not be run with all ODP implementations, it would be better if the test case did not rely on the optional feature. Tests with smallish sequence numbers do not really need the SA update feature, but potential future ESN tests probably need it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants