Tue 25 Aug 2015 05:35:25 AM UTC, comment #8:
Fixed by removing the defines. Plus I'll change the netif addresses to ip_addr_t.
Also, I've added bug #45818 (API functions should check if type of ip_addr_t parameter matches the pcb type).
|
Tue 12 May 2015 05:31:31 PM UTC, comment #7:
I think API will stay the same. As long as we use the API instead of accessing netif-> fields directly, we'll be fine.
Now I have to find time to actually do this...
|
Mon 11 May 2015 07:51:06 PM UTC, comment #6:
> I can now say that providing temporary storage addresses is becoming
> really annoying to work with.
Yeah, I can imagine that. When writing that code, I hadn't thought we would need that too often, but rather in one or two lines of the core code where interaction between UDP/TCP and IP is done. I'm fine with changing netif address types to achieve what you want.
However, my main goal here would be to maintain source code compatibility for old code, as much as possible. I.e. Neif address initialization should not have to be changed.
|
Wed 06 May 2015 04:49:41 PM UTC, comment #5:
I can now say that providing temporary storage addresses is becoming really annoying to work with.
Changing netif addresses to ip_addr_t should make things much better.
In general, IPv6 code has been quite good of using API to access addresses. So it should be an easy change there, just add an ip_2_ip6 to each macro.
IPv4 has more old code and doesn't use API, just &netif->ip_addr everywhere. So I will create a macro:
#define netif_ip4_addr(netif) ip_2_ip4(&(netif->ip_addr))
Same for netmask and gateway. I also wonder if I should change netif->ip_addr to netif->ip4_addr
Good news about this change, is that it should simplify UDP and TCP code as well. I haven't gone through all of it, but there are many places using temporary storage addresses that we could eliminate with this change. Especially those places where we select a source address for a PCB.
|
Tue 05 May 2015 09:43:31 PM UTC, comment #4:
Actually now I'm seeing that in some cases it is not cast directly (e.g. netconn_bind)
so I'm wondering if we should change addresses in netif to ip_addr_t, or I guess we need to suck it up and provide storage addresses when doing operations such as bind.
|
Tue 05 May 2015 07:02:13 PM UTC, comment #3:
Hmm, good point. OTOH, I also thought about making the netif addresses ip_addr_t, just to be able to use them as parameters to all the functions taking ip_addr_t pointers...
To be honest, I don't really know what's best right now. Why don't you go ahead an commit your changes and we'll see how it feels? The whole ip_addr_t change I did came out like that as well: I just was too annoyed of all that ipX crap that I had to do something :-)
|
Tue 05 May 2015 06:27:25 PM UTC, comment #2:
Another option is to keep the like they were originally. Here is what I mean:
Inside the API code, the ip_addr_t will be cast to either ip4_addr_t or ip6_addr_t. So far I don't see any check to the type field, there are just checks for the PCB type.
Sometimes I use e.g. netif addresses for API calls. These are ip4 or ip6 type. We'd need a temporary storage address in both cases. This will become a bit of a pain, and unnecessary since the ip_addr_t will be cast back to ip4_ or ip6_.
|
Tue 05 May 2015 06:23:00 PM UTC, comment #1:
Yep, seems like I have forgotten to remove them in api.h. I already removed some of the "overloaded" IPv6 function-like macros, e.g. in udp.
|
Tue 05 May 2015 06:16:38 PM UTC, original submission:
With the new ip_addr_t with type field, there are times where we must convert a version-specific address to a generic address.
e.g. netconn_bind_ip6() macro
None of these macros are using ip6_2_ip() correctly.
Possible fixes:
1) add a storage address parameter to the macros
2) remove the macros and force the user to supply a ip_addr_t parameter
Recommended fix: #2. Since the user will have to supply a ip_addr_t in either case, what's the point of keeping these macros?
I had originally added them when we didn't have the new ip_addr_t. I think they are obsolete now.
|