tasklwIP - A Lightweight TCP/IP stack - Tasks: task #14314, Implement interface name/index APIs

 
 

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

task #14314: Implement interface name/index APIs

Submitter:  Joel Cunningham <jcunningham>
Submitted:  Fri 20 Jan 2017 04:36:47 AM UTC
   
 
Category:  socket/netconn Should Start On:  Fri 20 Jan 2017 12:00:00 AM UTC
Should be Finished on:  Fri 20 Jan 2017 12:00:00 AM UTC Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  jcunningham Percent Complete:  90%
Open/Closed:  Closed Planned Release:  None
Effort:  0.00

Jump to the original submission

Thu 23 Feb 2017 11:50:40 PM UTC, comment #70: 

Great, thanks for wrapping up the remaining lose end!

Joel Cunningham <jcunningham>
Group Member
Thu 23 Feb 2017 07:44:03 PM UTC, comment #69: 

Fixed in a slightly different way. I copied your comment though (but forgot to mention that in the commit, sorry for that).

Soooo, this should really be done now :-)

Setting to 90% to indicate that if_nameindex (+free) are missing (if someone comes back here and checks).

Simon Goldschmidt <goldsimon>
Group administrator
Thu 23 Feb 2017 07:17:44 PM UTC, comment #68: 

No, I didn't. Too many comments here ;-)

Simon Goldschmidt <goldsimon>
Group administrator
Thu 23 Feb 2017 04:06:34 PM UTC, comment #67: 

Just in case you didn't see: I already added a patch to this thread for that.. :)

David van Moolenbroek <dcvmoole>
Thu 23 Feb 2017 04:04:54 PM UTC, comment #66: 

Right. I think we should add a uniqueness check to netif_add. The overhead should be ok for most use cases. I'll see if I can do this in the next days

Simon Goldschmidt <goldsimon>
Group administrator
Thu 23 Feb 2017 03:33:41 PM UTC, comment #65: 

Simon,

I'm fine with omitting them for now.

The only other thing I think mentioned in the comments was with PPP doing lots of netif_add/remove and that it will cause netif->num rollover

Joel Cunningham <jcunningham>
Group Member
Thu 23 Feb 2017 02:28:36 PM UTC, comment #64: 

Joel, if only if_nameindex (+free) are missing, shouldn't we just omit them until someone needs them?

In that case, we can close this as done, right? Or did I miss something in the 63 comments? :-)

Simon Goldschmidt <goldsimon>
Group administrator
Mon 20 Feb 2017 01:44:31 PM UTC, comment #63: 

FreeRTOS/MPU should support the MPU thing (and was the original motivation for it). I don't know how many people use this, but it should certainly work. I can't test it though as my STM32 don't seem to have an MPU :-(

Simon Goldschmidt <goldsimon>
Group administrator
Thu 16 Feb 2017 07:18:19 AM UTC, comment #62: 

Is there anyone out there using that feature (MPU support)? Does it still work? I never had an opportunity to test it.

Dirk Ziegelmeier <dziegel>
Group administrator
Thu 16 Feb 2017 07:17:23 AM UTC, comment #61: 

Is there anyone out there using that feature? Does it still work? I never had an opportunity to test it.

Dirk Ziegelmeier <dziegel>
Group administrator
Wed 15 Feb 2017 10:50:50 PM UTC, comment #60: 

My two cents are that MPU support is significant extra work when doing API feature work.  Almost to the point where making things work in MPU land was as much work as developing the feature.

If MPU support has low usage, maybe we should re-evaluate the cost vs benefit

Joel Cunningham <jcunningham>
Group Member
Wed 15 Feb 2017 07:50:02 PM UTC, comment #59: 

RE comment 56:
I keep forgetting that dumb MPU thingy. I don't know if it's worth the hassle. Better leave it as is now.

Although I would love to see a more generic solution for "do something in tcpip-thread and wait for completion" that does not involve adding structs, mem pools and defines...

Simon Goldschmidt <goldsimon>
Group administrator
Fri 10 Feb 2017 06:16:59 AM UTC, comment #58: 

Travis CI is showing a Unix port build failure with using errno in if_api.c.  I took at stab a fixing it, but am currently limited to only having my MSVC port running

Joel Cunningham <jcunningham>
Group Member
Fri 10 Feb 2017 05:03:03 AM UTC, comment #57: 

Couple of commits tonight:

  • Added sockets_priv.h.  Curently contains struct lwip_setgetsockopt_data and set_errno().
  • Added errno support to if_indextoname()
  • Minor clean ups in if_api.h


I also noticed there is a forward declaration of struct lwip_sock in sockets.h.  Is this leftover from something old and can now be removed?  Also if there is any other private socket symbols in sockets.h, feel free to move them

Joel Cunningham <jcunningham>
Group Member
Thu 09 Feb 2017 09:24:41 PM UTC, comment #56: 

Simon,

I could take that approach, it would mean defining a new API msg structure (struct if_apimsg?) with a memory pool (because of MPU support) so I can use tcpip_api_call() and also a new set of macros maping to API_VAR and such.  Is that preferable to leveraging netifapi?

Joel Cunningham <jcunningham>
Group Member
Thu 09 Feb 2017 08:36:40 PM UTC, comment #55: 

Re comment #45++:
I think David's proposal could be implemented if you go the direct way from the RFC-API file to netif.c. You'd need to ensure you do this in tcpip_thread, but I don't see a real need to provide all these functions in netifapi (plus the code size overhead). Even for the functions existing now, it might be good enough to just somehow get your "core-related" part run in the correct thread and remove the new "netifapi_netif_*" functions...

Just my feeling though...

Simon Goldschmidt <goldsimon>
Group administrator
Thu 09 Feb 2017 08:00:11 PM UTC, comment #54: 


> Also, if we continue to add more RFC-style (standardized) APIs, it might be worth adding yet another sub directory for those files to make it clear to everyone that these should implement a standard (in contrast to our own APIs like netconn, netbuf, netifapi, etc.)


I like this idea too!

> I'd vote for the _priv.h thing.


I'll give this a go

Joel Cunningham <jcunningham>
Group Member
Thu 09 Feb 2017 07:53:02 PM UTC, comment #53: 

I'd vote for the _priv.h thing.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 09 Feb 2017 07:51:38 PM UTC, comment #52: 

Would anyone be opposed to me moving the set_errno macro from sockets.c into sockets.h so I don't have to duplicate that logic for use in if_api.c?

Alternatively we could introduce a sockets_priv.h to separate internal socket API from external.  It looks like there is at least one instance of private types (struct lwip_setgetsockopt_data) already in sockets.h

Joel Cunningham <jcunningham>
Group Member
Thu 09 Feb 2017 07:47:12 PM UTC, comment #51: 

For this kind of new code (and given the few people discussing here), I think you can directly push if you think it's good. If we think differently, we can still change it later ;) Given the few developers sharing opinions here, lwip has evolved to be developed like that in the last years... I know it's much different than e.g. linux development...

> hopefully not duplicating any errno logic from sockets.c


In that case, we might want to share the code between the two files somewhere.

Also, if we continue to add more RFC-style (standardized) APIs, it might be worth adding yet another sub directory for those files to make it clear to everyone that these should implement a standard (in contrast to our own APIs like netconn, netbuf, netifapi, etc.)

Simon Goldschmidt <goldsimon>
Group administrator
Thu 09 Feb 2017 07:32:23 PM UTC, comment #50: 

Dirk,

Thanks for finding that.  Looks like only if_indextoname() has errors (errno behavior) defined.  I'll put together a patch, hopefully not duplicating any errno logic from sockets.c

Joel Cunningham <jcunningham>
Group Member
Thu 09 Feb 2017 06:31:56 PM UTC, comment #49: 

I just noticed that setting errno is missing in if_indextoname

http://pubs.opengroup.org/onlinepubs/009695399/functions/if_indextoname.html

Dirk Ziegelmeier <dziegel>
Group administrator
Thu 09 Feb 2017 04:00:00 PM UTC, comment #48: 

Ah fair enough, I don't know enough about the multithreading situation to comment on that.

David van Moolenbroek <dcvmoole>
Thu 09 Feb 2017 03:42:24 PM UTC, comment #47: 

David,

I'm fine with an iterator API, we would just need to ensure the list creation is atomic in regards to any simultaneous add/remove. 

Creating a list inside the LwIP context ensures this, but maybe a lighter-weight approach would be to use a generator that is incremented every time netif_list is modified and we rebuild the list if it changes during iteration.  That would be a new global int though

Joel Cunningham <jcunningham>
Group Member
Thu 09 Feb 2017 03:24:42 PM UTC, comment #46: 


> If I have a "netif" version of this structure[..] So I'm trying to think of an API that makes sense at the netif layer and can be easily re-used at the if_api layer


Just my 2c: isn't it a bit overkill to have a fullblown structure for this at the netif level at all? How about something like,

1) a netif iterator function:

   struct netif *
   netif_iter(struct netif *prev)
   {
     return ((prev == NULL) ? netif_list : (prev->next));
   }

   and,
2) a netif_get_name(netif, buf) function, which could share code with netif_index_to_name() ?

Any higher layers can construct a list in whatever form they wish using those two..

David van Moolenbroek <dcvmoole>
Thu 09 Feb 2017 03:13:51 PM UTC, comment #45: 

There are two additional APIs from RFC 3493: if_nameindex() and if_freenameindex().  These two APIs are for getting a list of all the name/index pairs and then freeing that list.

I've got an old prototype for these, but am having some trouble fitting it into the proper abstraction where standard types stay in if_api.c, with the logic for list allocation/creation happening in netif.  The list is an arrary of type:

struct if_nameindex {
     unsigned int   if_index;  /* 1, 2, ... */
     char          if_name;   / null terminated name: "le0", ... */
   };

If I have a "netif" version of this structure, if_index should be a u8_t since that's what the netif index APIs return, meaning that the structures won't be binary compatible.  So I'm trying to think of an API that makes sense at the netif layer and can be easily re-used at the if_api layer

All-in-all, this could be delayed if we have no determined use case.  It's a nice-to-have API that enumerates your interfaces rather than applications having to rely on knowing what the interface name is

Joel Cunningham <jcunningham>
Group Member
Thu 09 Feb 2017 11:21:37 AM UTC, comment #44: 

Joel, could you give me an update about the status here? I'm a bit lost figuring out what the remaining 50% are ;-)

Simon Goldschmidt <goldsimon>
Group administrator
Sun 05 Feb 2017 11:55:39 AM UTC, comment #43: 

Removed usage of netif->num in slipif.c

Dirk Ziegelmeier <dziegel>
Group administrator
Fri 03 Feb 2017 11:19:10 PM UTC, comment #42: 

include clean ups committed in: 4fb7d741657bbdd9e06759af55ca50710e6b4cba

Simon, should we open a separate bug for fixing netif_find() since that's been broken (for netif->num > 9) aside from this task?

I think then the netif->num uniqueness due to PPP is the only remaining cleanup, right?

Joel Cunningham <jcunningham>
Group Member
Fri 03 Feb 2017 09:43:24 PM UTC, comment #41: 


> Well, lwIP has the (unwritten?) rule that C files and their header files have the same base name


api.h is one exception that I see, but I'm happy to conform  since the compatibility headers also exist. I'll also make that change

Joel Cunningham <jcunningham>
Group Member
Fri 03 Feb 2017 09:36:54 PM UTC, comment #40: 

Well, lwIP has the (unwritten?) rule that C files and their header files have the same base name. There are some exceptions to that, I guess... (after all, it's just always been like this for lwIP :-)

Speaking of "include architecture", I do think it's bad that "core" and "api" headers are in the same folder, but as for lwIP, backwards compatibility is much like a sacred cow, I haven't changed that in all these years I'm active for lwIP now...

Simon Goldschmidt <goldsimon>
Group administrator
Fri 03 Feb 2017 09:35:15 PM UTC, comment #39: 

Also, I consider the header name more important because that's how an application developer interfaces with the API (and not the C file)

Joel Cunningham <jcunningham>
Group Member
Fri 03 Feb 2017 09:28:30 PM UTC, comment #38: 

The <net/if.h> name is mandated by RFC 3493 under section 7 (Summary of New Definitions)

I had already added the compatibility header <net/if.h> under the posix folder which includes lwip/if.h.  I was following the example of netdb.h which uses the same for the actual header and compatibility header.  The socket header is very close (lwip/sockets.h vs sys/socket.h)

My preference is for the lwip header stay close to the mandated name since the if_api.c file is intending to implement the RFC 3493 standard APIs (possibly other POSIX if APIs in the future).  I have worked with build environments which also dumped all headers into a global flattened list (terrible I know), but I'm not sure how common if.h is, maybe if the product had another network stack?

I'll go ahead and make the NETIF_NAMESIZE change and keep in mind the include architecture for future work :)



Joel Cunningham <jcunningham>
Group Member
Fri 03 Feb 2017 08:56:45 PM UTC, comment #37: 


> if.c renamed to if_api.c


Ehrm, shouldn't if.h be renamed to if_api.h, too, or isn't this the direct counterpart of if_api.c?

The NETIF_NAMESIZE thing sounds good. I just don't want to introduce includes in the wrong direction.


After your explanation, I got the idea behind it. However, netif_find() seems to only handle 10 netifs then...

Simon Goldschmidt <goldsimon>
Group administrator
Thu 02 Feb 2017 03:07:17 PM UTC, comment #36: 

if.c renamed to if_api.c in b6c995fed8a7783d06ab01fae314afe6e7d85b54

Joel Cunningham <jcunningham>
Group Member
Wed 01 Feb 2017 03:04:07 PM UTC, comment #35: 


> It seems to break the clean separation between APIs:
> - core should not include 'api' header files (thus netif.c should not include if.h and cannot access IF_NAMESIZE


I could move the define to netif.h or add a new define like NETIF_NAMESIZE (in netif.h) which is the canonical definition and then #define IF_NAMESIZE to it in if.h

> - the fact that an 'index' is 1-based and 0 is invalid is an RFC3493 definition. To me, the API provided by netif.h would better work on 'num' only and the conversion +/-1 to index should be done in if.c?


I put the main implementation of the index/name conversion in the netif APIs because David talked about needing to use the APIs from raw/core code and not the high level sockets API

> - same thing for netif name conversion, but this is a bit more strange as 'index_to_name' returns a long name but 'name_to_index' only checks the first 2 chars.


> --> The netif.h/.c functions should use 'num' instead of 'index' (do we need an 'invalud num' then?) and we might need to increase the size of struct netif's 'name' field to make index->name->index check the name correctly.


I'm not sure if I follow this completely.  netif_name_to_index relies on netif_find() to compare the input name to the netif name.  This function handles comparing the 2 chars and num.  In netif_index_to_name, we compare the index, then output the 2 chars and number.  IFNAMESZ is defined to exactly fit LwIP's name size which is 2 characters and a number (up to 3 digits, max of 255).  I didn't see a reason to increase the name storage in the netif structure to also include a string version of the num.  Is that what you were suggesting?

Joel Cunningham <jcunningham>
Group Member
Wed 01 Feb 2017 02:10:44 PM UTC, comment #34: 


> That's dumb anyway and should be removed :)


Hehe well sure - I guess my point is that making netif->num 1-based cannot be done without breaking the code of at least some lwIP users though. That also applies to other cases where users currently assign netif->num themselves. An added note in UPGRADING may be warranted either way.

David van Moolenbroek <dcvmoole>
Wed 01 Feb 2017 01:30:50 PM UTC, comment #33: 


> I believe this would break (at least) the slipif code


That's dumb anyway and should be removed :)

Simon Goldschmidt <goldsimon>
Group administrator
Wed 01 Feb 2017 01:20:50 PM UTC, comment #32: 


> Proposal: Make netif->num compliant to RFC -> no complicated mapping needed. Just let netif->num start at 1 and make 0 an invalid value.


As I noted in task #13106, I believe this would break (at least) the slipif code..

David van Moolenbroek <dcvmoole>
Wed 01 Feb 2017 01:19:25 PM UTC, comment #31: 


> To me, the API provided by netif.h would better work on 'num' only and the conversion +/-1 to index should be done in if.c?


It may be worth noting that my scopes patch relies on the netif calls doing the +1 conversion, as IP6_NO_ZONE (0) is a special value. As such there is a reason to have this convention in the core layer too, and so I would keep this as is.

> The question is how would David improve this for his many-netifs case?


Oh I don't have that many either; ten netifs is currently my maximum concurrent total. There may be a high turnover though, since my implementation supports dynamic pseudodevices - e.g. I have tests that dynamically create and destroy an additional loopback device for each subtest. As before: as long as I can override the value of 'num' from the init callback, I don't really mind what netif_add() sets it to (or how). If netif_add() guarantees uniqueness I can work with that as well, but I'm entirely happy with the way things are right now, too.

David van Moolenbroek <dcvmoole>
Wed 01 Feb 2017 01:11:57 PM UTC, comment #30: 

Proposal: Make netif->num compliant to RFC -> no complicated mapping needed. Just let netif->num start at 1 and make 0 an invalid value.

Dirk Ziegelmeier <dziegel>
Group administrator
Wed 01 Feb 2017 01:07:24 PM UTC, comment #29: 

Sorry for not seeing this earlier, but I have to comment on this change a bit:

It seems to break the clean separation between APIs:
- core should not include 'api' header files (thus netif.c should not include if.h and cannot access IF_NAMESIZE
- the fact that an 'index' is 1-based and 0 is invalid is an RFC3493 definition. To me, the API provided by netif.h would better work on 'num' only and the conversion +/-1 to index should be done in if.c?
- same thing for netif name conversion, but this is a bit more strange as 'index_to_name' returns a long name but 'name_to_index' only checks the first 2 chars.

--> The netif.h/.c functions should use 'num' instead of 'index' (do we need an 'invalud num' then?) and we might need to increase the size of struct netif's 'name' field to make index->name->index check the name correctly.


Coming back on PPP, the core code doesn't call netif_add() often, so that should be OK? OTOH, there might always be products that add/remove netifs at runtime (just because it works) and they would not see the limitation until someone tests it with >255 calls to netif_add(), so I think adding the uniqueness check to netif_add() would be OK. The question is how would David improve this for his many-netifs case?


As to the file name change, I think if_api.c is good.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 01 Feb 2017 12:38:21 PM UTC, comment #28: 

I'm unsure about this. Best solution would be the user has to specify a number at netif_add(), but that would be a breaking change.

On the other hand, a simple loop checking netif->num is not much code, and for systems with few netifs it is not much runtime overhead. So I think the code can be added to netif.c.

Last question: Do we need to guard it with yet-another-#ifdef? I think not, until someone complains :-)

Dirk Ziegelmeier <dziegel>
Group administrator
Tue 31 Jan 2017 08:53:56 PM UTC, comment #27: 

So far we've got a vote for if_api.c from Dirk.  This is also my preferred choice.  Any other input?

On the PPP netif add/remove usage, is there any additional discussion that would help determine whether the logic should go in netif.c?

Joel Cunningham <jcunningham>
Group Member
Mon 30 Jan 2017 08:11:00 PM UTC, comment #26: 

Sylvain,

good catch on PPP uniqueness. I'm with you that we should support systems adding/removing netifs at runtime (just like PPP does). I've always wondered how that could have worked, but netif->num just hasn't been used by the stack so far (it just existed as a struct netif member to be accessed by foreign code).

I'm not convinced this should be in the netif's init callback, although overriding it for speed might be necessary for some setups (are there setups with more than 256 netifs?). However, in that case the linked list of netifs becomes a slowdown factor in other parts of the code that are executed more often (e.g. routing on TX)...

Simon Goldschmidt <goldsimon>
Group administrator
Mon 30 Jan 2017 07:53:42 AM UTC, comment #25: 

I'd vote for if_api.c - maybe there is more that can fit into that file in the future

Dirk Ziegelmeier <dziegel>
Group administrator
Fri 27 Jan 2017 07:26:40 PM UTC, comment #24: 

For PPP the user can't change the init_cb netif callback. I will hit the netif_num overflows in one of "my" product where I am recycling a previously used PPP PCB for a GPRS backup link. (I will also hit it in PPP tests where I am creating/freeing PPP PCB for tests purposes but that's less important.)

Therefore I am willing to add the uniqueness loop inside the ppp_netif_init_cb function to prevent hitting it myself and having to patch lwIP… I agree we don't need to support systems with more than let's say 10 netifs but we should in my opinion support systems that are removing and adding netif on the fly. If there are only a few netifs the naive loop would do the job. (Well even with 256 netifs, looping 65536 times is meaningless actually).

The previous PPP implementation in 1.4.1 added the netif only when the link was up and removed it when it goes down, so that's a behavior which already existed in the stack itself. It was not wrong in my opinion, I only changed that in order to avoid bringing down a fully established link just because the netif could not be added, moving it to ppp_new allowed user to have an early notification about this failure.

Sylvain Rochet <gradator>
Group Member
Thu 26 Jan 2017 03:15:26 PM UTC, comment #23: 

I'm not opposed to that.  Ideally I'd like the name to convey that the file contains the RFC standard interface name/index APIs.  I followed how netdb.c and sockets.h are named after the header file.  A couple of ideas:

if_api.c
if_index.c
if_rfc3493.c

Joel Cunningham <jcunningham>
Group Member
Thu 26 Jan 2017 02:47:47 PM UTC, comment #22: 

Could we change the name of "if.c" to somewhat more lwip-named?

I'd like to prevent having to change this in the future when other libraries also have an object with that name (and dumb build environments like IAR put all objects into the same directory resulting in name clashes).

Simon Goldschmidt <goldsimon>
Group administrator
Fri 20 Jan 2017 09:00:36 PM UTC, comment #21: 

:) perfect, thanks again!

David van Moolenbroek <dcvmoole>
Fri 20 Jan 2017 08:59:22 PM UTC, comment #20: 

Committed in e158f87286512f5a6bba3c52bad06953e8eae6cd

Joel Cunningham <jcunningham>
Group Member
Fri 20 Jan 2017 08:41:11 PM UTC, comment #19: 

Cool, thank you :)

David van Moolenbroek <dcvmoole>
Fri 20 Jan 2017 08:40:40 PM UTC, comment #18: 

Ok thanks for clarifying that.  I'll push a new commit that moves the logic as per comment #3 and documents we also support netifs fully managing the number.  Sorry for the mis-understanding :)

Joel Cunningham <jcunningham>
Group Member
Fri 20 Jan 2017 08:37:27 PM UTC, comment #17: 

Yes :) Specifically I have an array of netif-containing structures, so I use an O(n) lookup to find a free one (a free list would make that O(1), even). Then I can set netif->num based on the array index. As such, in my use case, the netif_num value is completely unused, so it's also not a problem if it overflows or whatever, as long as I can set netif->num myself.

David van Moolenbroek <dcvmoole>
Fri 20 Jan 2017 08:36:17 PM UTC, comment #16: 

I just wanted to say lwIP should not try to resolve that case internally by adding logic that is bloat for "normal embedded systems". David's solution gives the user the opportunity to ensure unique netif numbers, so everything will just work in for the dynamic case.

Dirk Ziegelmeier <dziegel>
Group administrator
Fri 20 Jan 2017 08:31:20 PM UTC, comment #15: 

David,

So you are currently experience overflow in your use case and want to resolve it in your init callback, by possibly doing the O(n^2) walking of the netifs?

I did understand what you were saying, but thought Dirk's guidance was that cases where netif_num overflows are not supported.  The proposed change would give an allowance to overflow cases, but only if handled by the netif driver

Joel Cunningham <jcunningham>
Group Member
Fri 20 Jan 2017 08:24:38 PM UTC, comment #14: 

Would you be so kind as to move the new netif_add() assert to after the init() callback call though? As it is now, I can no longer override netif->num from the init() call because the assert will trigger first, so things actually got way worse for me this way.. See also the order I sketched in comment #3. Thanks :)

David van Moolenbroek <dcvmoole>
Fri 20 Jan 2017 08:22:11 PM UTC, comment #13: 

Patches committed in:

lwip: 9c80a6625344768572777accbe77ac968c10250f
1b20e664bfb93dc6a1dcc467103a4db7f1e24ede

lwip-contrib: 5ce470dcfb1148d0ed36acfece3c63a4f0dd5ddc

I'm going to leave this task open for additional implementation of if_nameindex and if_freenameindex.  The remaining part of my old patch has these too, but I've got to clean it up for LWIP_MPU_COMPATBILE support.  These APIs are a little more complicated because if_nameindex returns dynamic memory

Joel Cunningham <jcunningham>
Group Member
Fri 20 Jan 2017 06:21:32 PM UTC, comment #12: 

Just add it to Filelists.mk in lwIP main rep, and don't worry, I'll correct it if necessary.

Dirk Ziegelmeier <dziegel>
Group administrator
Fri 20 Jan 2017 06:14:52 PM UTC, comment #11: 

Triggering an LWIP_ASSERT when netif_num overflows is fine for me as well.  I can add that to my patches.

Dirk,

What else do I need to add in lwip-contrib to add if.c and if.h to other port's build systems?  I updated MSVC, but don't have any other environments setup.

Joel Cunningham <jcunningham>
Group Member
Fri 20 Jan 2017 05:59:01 PM UTC, comment #10: 

Yes sure!

David van Moolenbroek <dcvmoole>
Fri 20 Jan 2017 05:57:11 PM UTC, comment #9: 

If the LWIP_ASSERT is OK, then Joel can commit his patches, they are OK to me.

Dirk Ziegelmeier <dziegel>
Group administrator
Fri 20 Jan 2017 05:55:36 PM UTC, comment #8: 

Because this is supposed to be a lightweight stack, I'd vote for the LWIP_ASSERT David proposed. People who create/destroy lots of interfaces at runtime need to patch lwIP for that.

But David's last patch adds useless code for 99% of the systems (correct me if I'm wrong here). And adding yet-another-#ifdef is also not desirable for me.

Would the LWIP_ASSERT be OK for you and Joel?

Dirk Ziegelmeier <dziegel>
Group administrator
Fri 20 Jan 2017 04:58:35 PM UTC, comment #7: 

Basic implementation attached, which I hope will allow the rest to move forward as well here. :)

(file #39519)

David van Moolenbroek <dcvmoole>
Fri 20 Jan 2017 03:32:39 PM UTC, comment #6: 


> select the next number, walk list of netifs to ensure it's unique


Oh sorry yes, that is what I was referring to with "O(n^2) loops"

David van Moolenbroek <dcvmoole>
Fri 20 Jan 2017 03:03:33 PM UTC, comment #5: 

Thanks for the quick feedback and good catch on netif->num not being unique.  I wasn't thinking of this issue either because my use case has static netifs (added once during power up).  Also the boundary case of netif->num being 255 can't be converted to an index

The possibility of rollover reminds me of the same issue we have with ephemeral port selection; once the next port to be used is selected, we have to walk the list of pcbs to ensure it's already not in use.

We could employ the same logic in netif_add(), select the next number, walk list of netifs to ensure it's unique.  This should be less frequent than port selection, though since our number space is much smaller (u8_t compared to u16_t for ports) it's more conceivable that we will get closer to exhaustion.  Anyone know of use cases for large number of netifs with LwIP?

Joel Cunningham <jcunningham>
Group Member
Fri 20 Jan 2017 11:50:55 AM UTC, comment #4: 

..I should add that such an array of netif pointers would help speed up index-to-netif lookups during routing decisions.

David van Moolenbroek <dcvmoole>
Fri 20 Jan 2017 11:48:43 AM UTC, comment #3: 

Neat, Joel!

As for netif->num wrapping, honestly I was planning on just adding something like this to netif_add():

  netif->num = netif_num++;
  ..
  init(netif);
  ..
+ LWIP_ASSERT("Your implementation creates too many netifs for interface number auto-assignment, set netif->num from init() yourself", netif->num < 255);

..which would be just fine for me, but perhaps not for others? I don't really like O(n^2) loops especially if I'm going to override the result anyway, but it's probably the easiest way to implement unique IDs. I'd be happy to implement this. There are of course many other options, including an array of netif pointers, but then the user would need to define a LWIP_MAX_NETIFS. Thoughts, opinions?

David van Moolenbroek <dcvmoole>
Fri 20 Jan 2017 07:09:18 AM UTC, comment #2: 

/* Interface indexes always start at 1 per RFC 3493, section 4, num starts at 0 */
#define netif_num_to_index(netif)   ((netif)->num + 1)
#define netif_index_to_num(index)   ((index) - 1)

Converting netif->num to index unfortunately is not that easy, because netifs can be added and removed at runtime. netif->num increases and may wrap around, and may therefore not even be unique. This needs to be fixed first.

I frequently forget that they may be added/removed at runtime, e.g. in the IPv6 scopes discussion, because I use lwIP in systems where the netifs are created only once during startup.

Dirk Ziegelmeier <dziegel>
Group administrator
Fri 20 Jan 2017 04:53:39 AM UTC, comment #1: 

Attached patch adds the following APIs:

Interface Identification APIs from RFC 3493:
  * lwip_if_nametoindex (COMPAT macro if_nametoindex)
  * lwip_if_indextoname (COMPAT macro if_indextoname)

netifapi:
  * netifapi_netif_name_to_index
  * netifapi_netif_index_to_name

netif:
  * netif_name_to_index
  * netif_index_to_name
  * netif_num_to_index
  * netif_index_to_num

I think the implementation turned out fairly straight forward. RFC 3493 introduces a new header (net/if.h) containing API.  Top level APIs are in if.c

netifapi_ functions turned out a little clunkier than I wanted because they must return err_t due to NETIFAPI_VAR_ALLOC when LWIP_MPU_COMPATBILE is enabled

netif APIs were structured after standards API

I have also attached a lwip-contrib patch for adding some shell commands for testing and integrating into VS project files.  I'll need help updating the other ports

(file #39516, file #39517)

Joel Cunningham <jcunningham>
Group Member
Fri 20 Jan 2017 04:36:47 AM UTC, original submission:  

Implement the following top level APIs from RFC 3493: Basic Socket Interface Extensions for IPv6, Section 4: Interface Identification

if_indextoname
if_nametoindex
if_freenameindex
if_nameindex

Also implement equivalent raw/core APIs (netif_) for NO_SYS deployments

Joel Cunningham <jcunningham>
Group Member

 

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

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gradator (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by dcvmoole (Posted a comment)
  • -email is unavailable- added by dziegel (Posted a comment)
  • -email is unavailable- added by jcunningham (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 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-02-23 jcunningham StatusIn Progress Done
        Percent Complete50% 90%
        Open/ClosedOpen Closed
    2017-02-23 jcunningham StatusDone In Progress
        Percent Complete90% 50%
        Open/ClosedClosed Open
    2017-02-23 goldsimon StatusIn Progress Done
        Percent Complete50% 90%
        Open/ClosedOpen Closed
    2017-01-20 jcunningham Percent Complete0% 50%
    2017-01-20 dcvmoole Attached File- Added 0001-netif-ensure-that-netif-num-is-unique.patch, #39519
    2017-01-20 jcunningham Attached File- Added 0001-Task-14314-Add-interface-name-index-APIs.patch, #39516
        Attached File- Added 0001-Add-interface-support-to-shell-and-VS-project-file.patch, #39517

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code