Skip to content

Conversation

@masayuki2009
Copy link
Contributor

Summary

  • This PR fixes a part of usrsocktest errors. (i.e. errors in NoBlockRecv and BlockRecv)
  • Actually, the test cases checked sockaddr_in values which contain padding field, however, the field should be ignored because it contains random values.

Impact

  • This PR affects a usrsocktest application only.

Limitations / TODO

  • There still remain errors and need more investigation.

Testing

  • I tested this PR with spresense:wifi which I added usrsocktest application.

@xiaoxiang781216
Copy link
Contributor

@masayuki2009 how about remove Make.defs patch from this PR? so I can merge it.

@masayuki2009
Copy link
Contributor Author

@xiaoxiang781216 sorry, I added the file by mistake.

NOTE: Because sin_zero field is just a padding, so should be ignored.

Signed-off-by: Masayuki Ishikawa <Masayuki.Ishikawa@jp.sony.com>
@masayuki2009
Copy link
Contributor Author

@xiaoxiang781216 Sorry, I've just pushed with -f

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Feb 21, 2020

@masayuki2009 no problem, I have merged it, thanks for your patch.

@xiaoxiang781216 xiaoxiang781216 merged commit 10c715d into apache:master Feb 21, 2020
@lukegluke
Copy link
Contributor

It fix fails in NoBlockRecv and BlockRecv, yes, thanks!

By the way, when I was investigating this I encountered on this sentence in stackoverflow:

Its required by specification to clear sin_zero

I'm too far from an expert in net stack, and don't know about any specification, but maybe (just a thought) it would be more correct to nulling padding somewhere inside the net stack?

@masayuki2009
Copy link
Contributor Author

@lukegluke

By the way, when I was investigating this I encountered on this sentence in stackoverflow:

Thanks for the comment. I'll check the article.

@xiaoxiang781216
Copy link
Contributor

But I think the requirement is the application should zero out the input addr before pass to socket API to avoid the undefined behaviour? addr is an output argument in our case.

@lukegluke
Copy link
Contributor

But I think the requirement is the application should zero out the input addr before pass to socket API to avoid the undefined behaviour? addr is an output argument in our case.

It was the first thing I did trying fix it - clear input addr in usrsocktest before pass to socket API, but netstack insert random values in sin_zero itself anyway.

@masayuki2009
Copy link
Contributor Author

@lukegluke

It was the first thing I did trying fix it - clear input addr in usrsocktest before pass to socket API, >but netstack insert random values in sin_zero itself anyway.

Actually recvfrom_request() in usrsocktest_daemon.c sets the return values,
so we need to modify the above function as well, if we do not modify
network stack code.

@masayuki2009
Copy link
Contributor Author

@lukegluke

I'm too far from an expert in net stack, and don't know about any specification,
but maybe (just a thought) it would be more correct to nulling padding somewhere
inside the net stack?

As far as I checked the Linux kernel source code, you are right.
I think zero padding should be implemented in net/socket/recvfrom.c,
if the from address family is AF_INET and fromlen is larger than sockaddr_in size.

Anyway, I'll try to implement it and then should be reviewed by @patacongo.

@xiaoxiang781216
Copy link
Contributor

If so other similar function accept, getsockname getpeername... need the similar change too

@masayuki2009
Copy link
Contributor Author

@xiaoxiang781216

If so other similar function accept, getsockname getpeername... need the similar change too

Thanks for the comments. I understand.

@masayuki2009
Copy link
Contributor Author

I've just sent a new PR. apache/nuttx#370

@masayuki2009 masayuki2009 deleted the fix_usrsocktest_erros branch June 16, 2020 07:10
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.

3 participants