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

net: Add INET_ADDRSTRLEN and use it at least in net_config #9818

Closed

Conversation

pfalcon
Copy link
Contributor

@pfalcon pfalcon commented Sep 6, 2018

POSIX defines INET_ADDRSTRLEN and INET6_ADDRSTRLEN as max sizes of
textual form of IP addresses (including terminating NUL):
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/netinet_in.h.html

We already define INET6_ADDRSTRLEN, so it makes sense to define
INET_ADDRSTRLEN too.

But in parallel to those, we also have NET_IPV4_ADDR_LEN and
NET_IPV6_ADDR_LEN, and that's what most of the code uses. But it
doesn't make sense to have 2 parallel definitions, and it would
make sense to just switch to POSIX defines.

The caveat is that INET6_ADDRSTRLEN is defined to be 46, while
max size of native IPv6 is 40 (with NUL). That's because
INET6_ADDRSTRLEN accounts for IPv6-mapped IPv4 addresses, as
explained in
https://stackoverflow.com/questions/39443413/why-is-inet6-addrstrlen-defined-as-46-in-c

E.g.:

ffff:ffff:ffff:ffff:ffff:ffff:255.255.255.255

So, it doesn't feel easy to replace current NET_IPV6_ADDR_LEN with
INET6_ADDRSTRLEN, given that many such buffers are allocated on
stack.

I think that switching to POSIX names, just define e.g.
INET6_ADDRSTRLEN_SHORT and use that for the output buffers,
where native IPv6 addresses are guaranteed.

Signed-off-by: Paul Sokolovsky paul.sokolovsky@linaro.org

@pfalcon pfalcon added the DNM This PR should not be merged (Do Not Merge) label Sep 6, 2018
@pfalcon
Copy link
Contributor Author

pfalcon commented Sep 6, 2018

My another contribution towards "cleanup IP stack" epic (#8723). The commit message described the issue, a solution, problem with the solution, and proposed fix for it. If ACKed, I can update patch to use INET_ADDRSTRLEN/INET6_ADDRSTRLEN_SHORT consistently, and remove NET_IPV4_ADDR_LEN/NET_IPV6_ADDR_LEN.

@codecov-io
Copy link

codecov-io commented Sep 6, 2018

Codecov Report

Merging #9818 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9818   +/-   ##
=======================================
  Coverage   52.27%   52.27%           
=======================================
  Files         213      213           
  Lines       25962    25962           
  Branches     5596     5596           
=======================================
  Hits        13571    13571           
  Misses      10154    10154           
  Partials     2237     2237
Impacted Files Coverage Δ
subsys/net/lib/config/init.c 67.94% <ø> (ø) ⬆️
include/net/net_ip.h 69.07% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8250b3e...e3205ec. Read the comment docs.

Copy link
Collaborator

@ozhuraki ozhuraki left a comment

Choose a reason for hiding this comment

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

Would it make sense to go for all-POSIX way here, i.e. consistently use INET_ADDRSTRLEN, INET_ADDR6STRLEN, inet_ntop(), inet_pton() everywhere?

One step further would be to introduce additional inet_ntop4(), inet_ntop6() macros (which would use inet_ntop()) and use them in the networking that would serve the following:

  • There wouldn’t be a need to specify INET_ADDRSTRLEN/ INET_ADDR6STRLEN.
  • The wouldn’t be a need to specify destination size, these macros would do a size sanity check on the destination buffer.
  • Work with addresses would become safer, more compact and cleaner.

This way there would be POSIX compliance and convenience.

Introducing INET6_ADDRSTRLEN_SHORT is just needless complication, there are a few standard options here:

  • Defining it as 40, documenting and warning on IPv6 mapped IPv4 addresses inside inet_ntop/pton().
  • Defining it as 46 and implementing full POSIX compliance in inet_ntop(), inet_pton().

@pfalcon pfalcon changed the title net: AddINET_ADDRSTRLEN and use it at least in net_config net: Add INET_ADDRSTRLEN and use it at least in net_config Sep 7, 2018
POSIX defines INET_ADDRSTRLEN and INET6_ADDRSTRLEN as max sizes of
textual form of IP addresses (including terminating NUL):
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/netinet_in.h.html

We already define INET6_ADDRSTRLEN, so it makes sense to define
INET_ADDRSTRLEN too.

But in parallel to those, we also have NET_IPV4_ADDR_LEN and
NET_IPV6_ADDR_LEN, and that's what most of the code uses. But it
doesn't make sense to have 2 parallel definitions, and it would
make sense to just switch to POSIX defines.

The caveat is that INET6_ADDRSTRLEN is defined to be 46, while
max size of native IPv6 is 40 (with NUL). That's because
INET6_ADDRSTRLEN accounts for IPv6-mapped IPv4 addresses, as
explained in
https://stackoverflow.com/questions/39443413/why-is-inet6-addrstrlen-defined-as-46-in-c

E.g.:

ffff:ffff:ffff:ffff:ffff:ffff:255.255.255.255

So, it doesn't feel easy to replace current NET_IPV6_ADDR_LEN with
INET6_ADDRSTRLEN, given that many such buffers are allocated on
stack.

I think that switching to POSIX names, just define e.g.
INET6_ADDRSTRLEN_SHORT and use that for the output buffers,
where native IPv6 addresses are guaranteed.

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
@pfalcon pfalcon force-pushed the net-INET_ADDRSTRLEN branch from a61e148 to e3205ec Compare September 7, 2018 09:03
@@ -179,7 +179,10 @@ struct net_addr {
extern const struct in6_addr in6addr_any;
Copy link
Member

Choose a reason for hiding this comment

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

The commit message is way to long and describes things that are not relevant to this pr.
It should be enough to just say

POSIX defines INET_ADDRSTRLEN and INET6_ADDRSTRLEN as max sizes of
textual form of IP addresses (including terminating NUL):
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/netinet_in.h.html

We already define INET6_ADDRSTRLEN, so it makes sense to define
INET_ADDRSTRLEN too.

The rest of the story is irrelevant to this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rest of the story is irrelevant to this change.

It's relevant, if you take into account that this patch not just introduces INET_ADDRSTRLEN, but tries to replace NET_IPV*_ADDR_LEN with INET*_ADDRSTRLEN.

But I get a hint from you and @ozhuraki's comments that adding it should be split from an attempt to switch to it. Will prepare a new PR then.

@pfalcon
Copy link
Contributor Author

pfalcon commented Sep 7, 2018

@ozhuraki :

Would it make sense to go for all-POSIX way here, i.e. consistently use INET_ADDRSTRLEN, INET_ADDR6STRLEN, inet_ntop(), inet_pton() everywhere?

Well, we definitely strive for as wide as makes sense POSIX compliance (or compatibility probably better term). But it's gradual, stepwise process. In this case, it seems even this PR hastes too much, putting 2 separable changes into one PR. So, I split addition of INET_ADDRSTRLEN into #9850, as hinted by @jukkar, and let me consider your arguments re: usage of INET_ADDRSTRLEN/INET6_ADDRSTRLEN in more detail and reply a bit later.

@pfalcon
Copy link
Contributor Author

pfalcon commented Sep 10, 2018

@ozhuraki :

Introducing INET6_ADDRSTRLEN_SHORT is just needless complication

Well, I definitely agree it's a complication. Maybe relatively minor one, but which breaks the beauty of the proposed solution.

Defining it as 40, documenting and warning on IPv6 mapped IPv4 addresses inside inet_ntop/pton().

That's definitely an option, but I feel to shy propose that ;-). http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/netinet_in.h.html clearly says:

INET6_ADDRSTRLEN
    46. Length of the string form for IPv6. 

Yes, it's possible to go against the word of the standard to follow its spirit (after all, it provides this symbolic constant for a reason). But as I said, I don't feel like going that way now.

@pfalcon
Copy link
Contributor Author

pfalcon commented Sep 10, 2018

@ozhuraki:

Would it make sense to go for all-POSIX way here, i.e. consistently use INET_ADDRSTRLEN, INET_ADDR6STRLEN, inet_ntop(), inet_pton() everywhere?

I'm not aware of the requirement "to go for all-POSIX way". Specifically, we have a requirement of providing POSIX interface to user applications, but internally can use whatever makes sense. Of course, if internally we have the same as POSIX, modulo "syntactical" differences like names or params order, it makes sense to just use POSIX stuff directly. But if we can/need to take some shortcut/optimization wrt to POSIX, we can, and indeed that's what we do.

So, the aim of this PR was to get rid of 2 seemingly parallel pairs of INET_ADDRSTRLEN/INET6_ADDRSTRLEN vs NET_IPV4_ADDR_LEN/NET_IPV6_ADDR_LEN. As the discussion showed, there's no consensus that it will be beneficial, or about the exact way to do that. In this case, I guess it just makes sense to stay as it is now. So, I'd like to withdraw this PR, salvaging first useful parts of it as #9850 and #9869.

@pfalcon pfalcon closed this Sep 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Networking DNM This PR should not be merged (Do Not Merge)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants