buglwIP - A Lightweight TCP/IP stack - Bugs: bug #45029, Several macros use ip6_2_ip()...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #45029: Several macros use ip6_2_ip() without supplying a storage address

Submitter:  Ivan Delamer <idelamer>
Submitted:  Tue 05 May 2015 06:16:38 PM UTC
   
 
Category:  IPv6 Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

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).

Simon Goldschmidt <goldsimon>
Group administrator
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...

Ivan Delamer <idelamer>
Group Member
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.

Simon Goldschmidt <goldsimon>
Group administrator
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.

Ivan Delamer <idelamer>
Group Member
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.

Ivan Delamer <idelamer>
Group Member
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 :-)

Simon Goldschmidt <goldsimon>
Group administrator
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_.


Ivan Delamer <idelamer>
Group Member
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.

Simon Goldschmidt <goldsimon>
Group administrator
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.

Ivan Delamer <idelamer>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by idelamer (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-08-25 goldsimon StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code