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

socket/SOL: sync the SOL index with linux #1291

Merged
merged 1 commit into from
Jun 29, 2020

Conversation

anchao
Copy link
Contributor

@anchao anchao commented Jun 24, 2020

Summary

socket/SOL: sync the SOL index with linux

Testing

CI check is enough

Change-Id: I32b9eb7cc3bc1428f0ff7bf5e60d7fff52621db2
Signed-off-by: chao.an <anchao@xiaomi.com>
@acassis
Copy link
Contributor

acassis commented Jun 25, 2020

@PetervdPerk-NXP Could you please review?

Copy link
Contributor

@PetervdPerk-NXP PetervdPerk-NXP left a comment

Choose a reason for hiding this comment

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

Looks good to me, psock_getsockopt works with a switch case so shouldn't pose any problems.

@yamt
Copy link
Contributor

yamt commented Jun 26, 2020

@anchao is there any benefit to be "sync" with linux?

@yamt
Copy link
Contributor

yamt commented Jun 26, 2020

i suspect dense numbers are easier for compilers to optimize switch statements.

@anchao
Copy link
Contributor Author

anchao commented Jun 27, 2020

@anchao is there any benefit to be "sync" with linux?

Unsynchronized options will cause some compatibility issues,
for example:

setsockopt(sock, IPPROTO_IP, IP_ADD_MEMBERSHIP, &mreq, sizeof(mreq));

int psock_setsockopt(FAR struct socket *psock, int level, int option,
                     FAR const void *value, socklen_t value_len)
...
      case SOL_IP:    <-- 
        ret = ipv4_setsockopt(psock, option, value, value_len);
        break;
....

this option configure will be failure since IPPROTO_IP and SOL_IP are different definitions in NuttX,
if we want to be fully compatible with posix applications, the experience of adopting Linux will be a good choice.

@yamt
Copy link
Contributor

yamt commented Jun 29, 2020

ok. sync with IPPROTO_ makes sense.
actually, shouldn't we just remove all SOL_ constants except SOL_SOCKET, and use IPPROTO_ constants instead?

@anchao
Copy link
Contributor Author

anchao commented Jun 29, 2020

ok. sync with IPPROTO_ makes sense.
actually, shouldn't we just remove all SOL_ constants except SOL_SOCKET, and use IPPROTO_ constants instead?

Done, please help to review the following PR:
#1320
apache/nuttx-apps#311

@jerpelea
Copy link
Contributor

LGTM

@xiaoxiang781216 xiaoxiang781216 merged commit b296adc into apache:master Jun 29, 2020
@anchao anchao deleted the 20062405 branch August 14, 2020 04:19
@btashton btashton added this to To-Add in Release Notes - 10.0.0 Oct 14, 2020
@btashton btashton moved this from To-Add to Minor in Release Notes - 10.0.0 Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants