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

fabtests/shared.c: Check if fi_info is returned correctly in case of FI_CONNREQ #9355

Merged
merged 2 commits into from
Oct 3, 2023

Conversation

Juee14Desai
Copy link
Contributor

This commit adds validation that the fi_info returned with an FI_CONNREQ
event is filled out properly with valid fields. The fi_info should be
similar to the fi_info that is used to open passive endpoint.

fabtests/common/shared.c Outdated Show resolved Hide resolved
fabtests/common/shared.c Outdated Show resolved Hide resolved
fabtests/common/shared.c Outdated Show resolved Hide resolved
fabtests/common/shared.c Outdated Show resolved Hide resolved
fabtests/common/shared.c Outdated Show resolved Hide resolved
@Juee14Desai
Copy link
Contributor Author

Sockets msg ep tests are failing. Would this validation not work for sockets provider?

@shefty
Copy link
Member

shefty commented Sep 20, 2023

Hmm... You may need to break apart the if statements and print an error to identify what portion of the check failed. The checks should work for any provider. If sockets is failing, can you identify why? Maybe the provider isn't correct.

@Juee14Desai
Copy link
Contributor Author

Hmm... You may need to break apart the if statements and print an error to identify what portion of the check failed. The checks should work for any provider. If sockets is failing, can you identify why? Maybe the provider isn't correct.

For sockets it looks like the prov_name is NULL and the api versions of info->fabric_attr (0) and fi_pep->fabric_attr (65554) do not match.

@shefty
Copy link
Member

shefty commented Sep 21, 2023

That sounds like bugs in the provider. We may need to add a fix to it as part of this PR for the tests to pass.

@shefty
Copy link
Member

shefty commented Sep 21, 2023

It does look like the sockets provider is not setting those fields correctly. prov_name is left as NULL. And I think abi_version isn't set either. The good news is that this means the test is working. :)

…FI_CONNREQ

This commit adds validation that the fi_info returned with an FI_CONNREQ

event is filled out properly with valid fields. The fi_info should be

similar to the fi_info that is used to open passive endpoint.

Signed-off-by: Juee Himalbhai Desai <[email protected]>
@Juee14Desai
Copy link
Contributor Author

AWS CI failed.

@shijin-aws
Copy link
Contributor

shijin-aws commented Sep 28, 2023

It's a bug in our CI infra code. We will rerun the test after it's fixed.

@shijin-aws
Copy link
Contributor

bot:aws:retest

1 similar comment
@wenduwan
Copy link
Contributor

bot:aws:retest

@Juee14Desai
Copy link
Contributor Author

Hi @shefty, Please let me know if there are any more comments.

prov/sockets/src/sock_ep.c Outdated Show resolved Hide resolved
prov/sockets/src/sock_ep.c Show resolved Hide resolved
src/fabric.c Outdated Show resolved Hide resolved
…truct

The api version and provider name returned in fi_info struct are not

set correctly in sock_ep.c.

Also, info struct is set to fi_dupinfo(hints) to allocate info struct

correctly.

Signed-off-by: Juee Himalbhai Desai <[email protected]>
@Juee14Desai
Copy link
Contributor Author

Intel CI error is unrelated to this change.

@Juee14Desai Juee14Desai requested a review from shefty October 2, 2023 23:43
@shefty shefty merged commit 7cdce37 into ofiwg:main Oct 3, 2023
@shefty
Copy link
Member

shefty commented Oct 3, 2023

thanks!

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.

4 participants