IP address parsing refactoring #1198

jukkar · 2017-08-20T20:07:53Z

As there is more uses for IP address parsing, create a generic net_ipaddr_parse() function for it and replace current functionality in DNS resolver by it. Update the unit tests to test the parsing function.

pfalcon · 2017-08-21T07:30:09Z

My understanding is that this function is intended for usage convenience first of all, then why:

@param str_len Length of the string to be parsed.

Why not just take null-terminated string?

Especially that there's code like:

ptr = strstr(str, "]");

This won't work properly with given length (e.g. if we want to parse address is a larger buffer, it may find "]" outside the string we want to parse).

pfalcon · 2017-08-21T07:33:40Z

subsys/net/ip/utils.c

Couldn't this port parsing be rewritten somehow to be common for both IPv6 & IPv4, instead of being duplicated?

Perhaps, I did not see too much reason to create a function for this as it is just couple of lines.

pfalcon · 2017-08-21T07:35:38Z

include/net/net_ip.h

I'd say this should specify explicitly what will be on sockaddr if port is not specified (e.g. port field set to 0, or yet better not touched, to let the caller to initialize it to default value prior to call).

Sure, that makes sense.

pfalcon · 2017-08-21T07:51:21Z

@jukkar: Also, as DNS was mentioned, pls have a look at GH-3992 when you get a chance (implementing that definitely goes to 1.10).

jukkar · 2017-08-21T09:34:48Z

Why not just take null-terminated string?

If we want to parse address from middle of a string. This way we do not need to copy to temporary buffer and parse that.

jukkar · 2017-08-21T09:35:44Z

This won't work properly with given length (e.g. if we want to parse address is a larger buffer, it may find "]" outside the string we want to parse).

The lengths are checked later in the function. I have unit tests that should check this.

jukkar · 2017-08-21T09:55:41Z

The lengths are checked later in the function. I have unit tests that should check this.

After checking the unit tests, I did not actually test this condition. I will send a new version that addresses this issue.

jukkar · 2017-08-21T10:41:32Z

Also, as DNS was mentioned, pls have a look at GH-3992 when you get a chance (implementing that definitely goes to 1.10).

As I commented in bug, we should fix the DNS resolver. One simple solution we could use, is to call this new net_ipaddr_parse() in DNS resolver, and if that fails, then try to resolve from DNS server.

jukkar · 2017-08-22T18:00:27Z

@pfalcon Added a new patch to this set that fixes the numeric DNS query issue you reported.

pfalcon

Thanks for the changes and adding numeric addresses support to DNS query! This fails CI so, but let me proactively approve this, as I'm going on vacation the next day.

lpereira · 2017-08-22T21:24:22Z

subsys/net/ip/utils.c

errno has to be set to 0 before calling strtol(). Might be a good idea to use strtoul() instead since it's unsigned anyway.

It looks like we do not need errno when checking the conversion failures if we use endptr as suggested in strtoul() manual page.

lpereira · 2017-08-22T21:25:51Z

subsys/net/ip/utils.c

While int and long are the same size, it's better to use the same return type for strtol() here: long int.

lpereira · 2017-08-22T21:26:34Z

subsys/net/ip/utils.c

If using strtoul() instead, you might change the range check with a cast: if ((unsigned long)(unsigned short)tmp != tmp). This often generates tighter code.

lpereira · 2017-08-22T21:28:12Z

subsys/net/ip/utils.c

Please use memchr() here: since there's a str_len parameter, a user might pass a non-zero-delimited string. Moreover, strstr() is overkill to scan for a single character.

Indeed, we can also then remove the second ptr check in next line.

lpereira · 2017-08-22T21:29:13Z

subsys/net/ip/utils.c

Nitpick: Why not a for (count = i = 0; str[i]; i++) ... instead?

Yes, and "for (count = i = 0; str[i] && i < str_len; i++) {" is even better as it checks the string length too.

lpereira · 2017-08-22T21:33:58Z

subsys/net/ip/utils.c

Are these always servers? Wouldn't host be better?

Left over when the code was in DNS resolve library, I will convert to "host" instead.

jukkar · 2017-08-23T09:58:35Z

New version which adds more unit tests and updates the code according to comments.

jukkar · 2017-08-29T20:26:54Z

@lpereira could you review / re-approve the changes?

lpereira

Could net_ipaddr_parse() be either split into smaller functions, with one per configuration variant, or at least split between parse_ipv4 and parse_ipv6 functions? The amount of copy-paste in parsing code worries me a bit.

lpereira · 2017-08-29T23:07:14Z

subsys/net/ip/utils.c

This passed from the other review, but strnlen() should be used here, since str is not guaranteed to have a NUL terminator. Other uses of strlen() here should be changed. Apparently, minimal libc does not have a strnlen() implementation, but it's trivial to add one.

lpereira · 2017-08-29T23:13:44Z

subsys/net/ip/utils.c

Do we have alloca() support? If so, these things copying str into ipaddr could become just a call to strndupa(). Soletta has a simple implementation that you can borrow (same license, Intel owns the copyright).

Lets not introduce yet another dependecy here :)

jukkar · 2017-08-30T10:03:24Z

Could net_ipaddr_parse() be either split into smaller functions, with one per configuration variant, or at least split between parse_ipv4 and parse_ipv6 functions? The amount of copy-paste in parsing code worries me a bit.

Yes, that makes sense. I sent a new version that does that. Also added more test cases to unit tests.

The net_ipaddr_parse() will take a string with optional port number and convert its information into struct sockaddr. The format of the IP string can be: 192.0.2.1:80 192.0.2.42 [2001:db8::1]:8080 [2001:db8::2] 2001:db::42 Signed-off-by: Jukka Rissanen <[email protected]>

Add IPv4 and IPv6 address parsing tests. Signed-off-by: Jukka Rissanen <[email protected]>

This commit removes IP address parsing from DNS init and replaces it by call to net_ipaddr_parse(). Signed-off-by: Jukka Rissanen <[email protected]>

If the query name is already in numeric format, there is no need to send the query string to DNS server as we can just convert it ourselves. Jira: ZEP-2562 Signed-off-by: Jukka Rissanen <[email protected]>

…hyrproject-rtos#1198) Signed-off-by: Cui Yan <[email protected]>

jukkar added the net label Aug 20, 2017

jukkar self-assigned this Aug 20, 2017

jukkar requested review from pfalcon and tbursztyka August 20, 2017 20:07

jukkar force-pushed the ipaddr-parse branch from 6665bd5 to d45fdeb Compare August 20, 2017 20:43

jukkar mentioned this pull request Aug 21, 2017

Add LwM2M firmware push/pull function #1049

Closed

pfalcon reviewed Aug 21, 2017

View reviewed changes

jukkar force-pushed the ipaddr-parse branch from d45fdeb to 0574b9f Compare August 21, 2017 10:34

jukkar added this to the v1.9 milestone Aug 22, 2017

jukkar force-pushed the ipaddr-parse branch from 0574b9f to b8e11be Compare August 22, 2017 17:57

pfalcon previously approved these changes Aug 22, 2017

View reviewed changes

jukkar dismissed pfalcon’s stale review via 1269a49 August 22, 2017 19:29

jukkar force-pushed the ipaddr-parse branch from b8e11be to 1269a49 Compare August 22, 2017 19:29

lpereira suggested changes Aug 22, 2017

View reviewed changes

jukkar force-pushed the ipaddr-parse branch from 1269a49 to 0b7abe8 Compare August 23, 2017 09:55

jukkar force-pushed the ipaddr-parse branch from 0b7abe8 to 1df2678 Compare August 28, 2017 07:38

jukkar mentioned this pull request Aug 28, 2017

Websocket support #1271

Closed

jukkar force-pushed the ipaddr-parse branch 2 times, most recently from f85f840 to cf2ee42 Compare August 29, 2017 19:51

lpereira suggested changes Aug 29, 2017

View reviewed changes

jukkar force-pushed the ipaddr-parse branch from cf2ee42 to 848ebfd Compare August 30, 2017 10:02

jukkar force-pushed the ipaddr-parse branch from 848ebfd to 7fe98d7 Compare August 30, 2017 12:30

jukkar added 4 commits August 30, 2017 16:06

net: tests: Add tests for IP address parsing

00ea1c5

Add IPv4 and IPv6 address parsing tests. Signed-off-by: Jukka Rissanen <[email protected]>

net: dns: Use IP address parsing function in init

6b76326

This commit removes IP address parsing from DNS init and replaces it by call to net_ipaddr_parse(). Signed-off-by: Jukka Rissanen <[email protected]>

net: dns: Do not resolve numeric IP address

99ab90b

If the query name is already in numeric format, there is no need to send the query string to DNS server as we can just convert it ourselves. Jira: ZEP-2562 Signed-off-by: Jukka Rissanen <[email protected]>

jukkar force-pushed the ipaddr-parse branch from 7fe98d7 to 99ab90b Compare August 30, 2017 13:07

lpereira approved these changes Aug 30, 2017

View reviewed changes

nashif merged commit bd425e6 into zephyrproject-rtos:master Aug 31, 2017

This was referenced Sep 18, 2017

net/dns: dns_get_addr_info() cannot "resolve" (parse) numeric IP addresses zephyriot/zep-jira14#2392

Closed

net/dns: dns_get_addr_info() cannot "resolve" (parse) numeric IP addresses zephyriot/zephyr-issues#2392

Closed

zephyrbot mentioned this pull request Sep 23, 2017

net/dns: dns_get_addr_info() cannot "resolve" (parse) numeric IP addresses #3992

Closed

nashif modified the milestones: v1.9, v1.9.0 Oct 3, 2017

nagineni pushed a commit to nagineni/zephyr that referenced this pull request Nov 20, 2017

[net] Update tcp client to add 'enter' for finished transmission (zep…

fee4637

…hyrproject-rtos#1198) Signed-off-by: Cui Yan <[email protected]>

jukkar deleted the ipaddr-parse branch June 21, 2018 06:38

IP address parsing refactoring #1198

IP address parsing refactoring #1198

Uh oh!

Conversation

jukkar commented Aug 20, 2017

Uh oh!

pfalcon commented Aug 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfalcon commented Aug 21, 2017 • edited by zephyrbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jukkar commented Aug 21, 2017

Uh oh!

jukkar commented Aug 21, 2017

Uh oh!

jukkar commented Aug 21, 2017

Uh oh!

jukkar commented Aug 21, 2017 • edited by zephyrbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jukkar commented Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pfalcon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jukkar commented Aug 23, 2017

Uh oh!

jukkar commented Aug 29, 2017

Uh oh!

lpereira left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jukkar commented Aug 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pfalcon commented Aug 21, 2017 •

edited by zephyrbot

Loading

jukkar commented Aug 21, 2017 •

edited by zephyrbot

Loading

jukkar commented Aug 22, 2017 •

edited

Loading