patchlwIP - A Lightweight TCP/IP stack - Patches: patch #6370, Sending to myself

 
 

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

patch #6370: Sending to myself

Submitter:  Luca Ceresoli <lucaceresoli>
Submitted:  Fri 11 Jan 2008 02:04:06 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  goldsimon Open/Closed:  Closed
Planned Release:  None

Jump to the original submission

Thu 19 Jun 2008 04:28:37 PM UTC, comment #24: 

I fixed the include and removed loop_cnt_max (using the define to test for max), so this should be done now.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 18 Jun 2008 07:26:37 PM UTC, comment #23: 

The include change is correct, I messed that up.

loop_cnt_max was intended to change per netif (have a macro to change this), but maybe we don't need that...

Simon Goldschmidt <goldsimon>
Group administrator
Wed 18 Jun 2008 03:31:45 PM UTC, comment #22: 

Tested and working! (NO_SYS=1)

Just a couple of very minor issues:

> --- netif.c 2008/06/12 20:10:09 1.66
> +++ netif.c 2008/06/17 19:39:22 1.67
> @@ -47,6 +47,8 @@
>  #include "netif/etharp.h"
>  #if ENABLE_LOOPBACK && !LWIP_NETIF_LOOPBACK_MULTITHREADING
>  #include "lwip/sys.h"
> +#else /* ENABLE_LOOPBACK && !LWIP_NETIF_LOOPBACK_MULTITHREADING */
> +#include "lwip/tcpip.h"
>  #endif /* ENABLE_LOOPBACK && !LWIP_NETIF_LOOPBACK_MULTITHREADING */

 
tcpip.h would be included when ENABLE_LOOPBACK==0 and LWIP_NETIF_LOOPBACK_MULTITHREADING==0.
Also, the protect macros in sys.h are required for ENABLE_LOOPBACK, any NO_SYS value.

I think it should be:
#if ENABLE_LOOPBACK
#include "lwip/sys.h"
#if LWIP_NETIF_LOOPBACK_MULTITHREADING
#include "lwip/tcpip.h"
#endif /* LWIP_NETIF_LOOPBACK_MULTITHREADING */
#endif /* ENABLE_LOOPBACK */

> --- netif.h 2008/06/12 20:10:10 1.44
> +++ netif.h 2008/06/17 19:39:22 1.45
> ...
> +#if LWIP_LOOPBACK_MAX_PBUFS
> +  u16_t loop_cnt_max;
> +  u16_t loop_cnt_current;
> +#endif /* LWIP_LOOPBACK_MAX_PBUFS */


Why a variable for the max value? It will nevere be changed after init.

Luca Ceresoli <lucaceresoli>
Tue 17 Jun 2008 07:42:53 PM UTC, comment #21: 

I just changed the loopback code a little: Basically, the multithreading environment now behaves much the same like the
non-OS environment (packets are put on a list in the netif). The only real difference is that the multithreading environment
schedules a call to netif_poll for that netif using tcpip_callback(), which has to be done manually in the non-OS
environment.

Also, I introduced a new setting, LWIP_LOOPBACK_MAX_PBUFS, which can limit the maximum number of pbufs queued for
loopback for each netif.

I even found the time to test it for both NO_SYS settings :-)

Simon Goldschmidt <goldsimon>
Group administrator
Mon 16 Jun 2008 03:26:50 PM UTC, comment #20: 

What you say is true of course. But now that we know exactly where netif_loop_output() is called (only from ip.c), I think we can safely call ip_input() from netif_poll().

Only netif_loop_output() may NOT call ip_input directly as this would lead to re-entering the tcp code (which is not supported and the reason the LOOPIF_MULTITHREADING setting was originally introduced). Since tcpip_input() cannot be used (it would call netif->input()), I think we have to use tcpip_callback somehow. I'll think about it.

Anyway, thanks for the feedback!

Simon Goldschmidt <goldsimon>
Group administrator
Mon 16 Jun 2008 10:43:55 AM UTC, comment #19: 


> I'm not sure if I caught all bugs
> (not that there would be any...:)

There are. :-[

We are forwarding at the IP level, so we don't prepend any link-level header. But netif->input() (ethernet_input() for me) requires such a header.

I think netif_poll() should forward up this way:
-      if(netif->input(in, netif) != ERR_OK) {
+      if(ip_input(in, netif) != ERR_OK) {

The same should be in netif_loop_output() (see attached patch) but I have no threads so I didn't test that.

Luca


(file #15851)

Luca Ceresoli <lucaceresoli>
Thu 12 Jun 2008 08:18:01 PM UTC, comment #18: 


> Is it even going to be finished in CVS? ;>


It IS!!

I made a slight modifications:
- netif_poll_all() polls all netifs (hehe)
- netif_poll(struct netif*) polls one specific netif

What's left to do (I'd suggest):
- limiting the number of packets (or pbufs?) on the queue of one netif
- limiting the number of packets (or pbufs?) queued for ALL netifs

e.g. one may queue 10 packets, all summed up may queue 20 packets...

Any suggestions?

Oh, and Luca: would you mind testing (i.e. using) this? As you might have noticed I'm a little short of lwIP-time lately and I'm not sure if I caught all bugs (not that there would be any...:)

Simon Goldschmidt <goldsimon>
Group administrator
Tue 08 Apr 2008 04:16:12 PM UTC, comment #17: 

I'm so sorry I haven't answered yet, but I just couldn't find the time to work on it...

In general, I think we agreed to including this into the stack. I'll check it in (and maybe change it in some places) when I find the time (maybe this weekend).

Of course any of the other developers is welcome to do this if they want to!

Simon Goldschmidt <goldsimon>
Group administrator
Tue 08 Apr 2008 02:44:32 PM UTC, comment #16: 

I'll recap what this patch is, so hopefully we may reach a final merge/reject decision.

This patch moves most of the code from loopif.c into netif.c to give all interfaces the "loopback" feature: an outgoing IP packet destined to the IP address of a netif is looped back up the stack and received again on that netif.

This feature is found on "heavyweight" stacks, but many lwIP users won't need it, so it's off by default.

The code implementing the loopback feature is activated when (LWIP_HAVE_LOOPIF || LWIP_NETIF_LOOPBACK). This maintains backward compatibility with current loopif.c.

The decision is taken in ip_output_if(): if a packet's destination IP address is equal to the netif->ip_addr it's looped back, otherwise nefit->output() is called as usual.

As inherited from loopif.c, this feature works both with and without multithreading. In the latter case packets to be looped back are enqueued and sent back up the stack when the netif_poll() function is called.

Additional features proposed by Simon in comment #9, that we kind of agreed to implement in a second step:

  • poll function for one specific netif, in addition to netif_poll() which checks all
  • a better check for multithreading (assert if input is ip_input? what else can be done to make this safe?) (I'll need support on this, since my environment does not support multithreading)
  • this is new, even for loopif: limit the packets on such a queue to prevent using all pbufs for loopback communication (could be turned off)


Luca Ceresoli <lucaceresoli>
Tue 01 Apr 2008 06:58:33 PM UTC, comment #15: 

Sorry guys, I really seem to have forgotten most of this (well, it's been a while :)...

I'll try to find the time to catch up with what I wrote below and check in the patch (we had some kind of consensus, after all).

I'd set the planned release of this to 1.4.0 (which doesn't prevent some things going in to 1.3.1, but maybe not everything), but it this version isn't listed yet.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 31 Mar 2008 01:13:20 PM UTC, comment #14: 

I haven't gone back carefully through the old comments to compare against what's been done, but I think it looks reasonable. Simon?

Jonathan Larmour <jifl>
Group Member
Mon 31 Mar 2008 08:31:31 AM UTC, comment #13: 

Now that 1.3.0 is out, time to recap with this patch.

The latest (file #14825) gracefully applies to 1.3.0.
It's working in my environment (raw API, NO_SYS=1, no multithreading).
With multithreading enabled in lwipopts.h, it's ok as far as I could go, i.e. compiling and linking.

I couldn't propose a cleaner way to maintain loopif compatibility than the ENABLE_LOOPBACK define and following lines at the beginning of netif.c, so I guess that will be ok.

Luca Ceresoli <lucaceresoli>
Fri 18 Jan 2008 10:23:00 AM UTC, comment #12: 

Here is my new patch, I just renamed some defines and variables as discussed here.
Given the 1.3.0 freeze, all the ugly backward-compat directives are still there. :(

The patch is against -rc1.

> What I'd like to see

I agree with all of the three, and also with comment #10: better adding them after the current patch is merged.


(file #14825)

Luca Ceresoli <lucaceresoli>
Thu 17 Jan 2008 07:40:27 PM UTC, comment #11: 

Right. And it's a pity this can't go into 1.3.0, but on the other hand, it's good to know it is finally coming!

Simon Goldschmidt <goldsimon>
Group administrator
Thu 17 Jan 2008 05:46:39 PM UTC, comment #10: 

It looks like trunk has frozen for 1.3.0 so the discussion about forward compatibility becomes irrelevant!

Simon, if I understand your comment #9 correctly, the first parts are changes that should be integrated into the patch before its committed, but the stuff beginning "What I'd like to see" are things which are nice but wouldn't stop the patch going in (after 1.3.0), is that right? I just want to make sure everyone's on the same page, and if a new task needs to be created for those after the patch goes in, then we know to do it.

Jonathan Larmour <jifl>
Group Member
Wed 16 Jan 2008 06:56:27 PM UTC, comment #9: 


> Some ideas to improve naming and the rest fo my patch.
> - IP_TO_SELF >> LWIP_NETIF_LOOBPACK


LOOPBACK, but OK

> - IP_TO_SELF_MULTITHREADING >> LWIP_NETIF_LOOBPACK_MULTITHREADING


ditto

> (argh, very long)


who cares? ;-)

> - netif.self_{first|last} >> netif.loop_{first|last}


OK for me

> - loopif_poll() >> no idea (Simon?)


no idea, really. Maybe netif_poll_loopback() ?

> - LWIP_LOOPIF_MULTITHREADING: will it be needed anymore, in case
> the loop behaviour is moved into netif?


No, it won't since loopif_poll and loopif_output can be removed.

> - How about setting the default value of
> IP_TO_SELF_MULTITHREADING to (!NO_SYS)?


A good default value.

> - Moving the _output() function from loopif.c to netif.c, I changed
> from pbuf_alloc(PBUF_RAW, ...) to pbuf_alloc(PBUF_LINK, ...). I need
> an ack/nack on that by someone with more knowledge of pbufs.


That has already been changed in CVS HEAD an is OK (I guess it was a copy & paste error while copying from an ETH netif).

What I'd like to see
- poll function for one specific netif and one to poll all netifs in netif_list.
- a better check for multithreading (assert if input is ip_input? what else can be done to make this safe?)
- this is new, even for loopif: limit the packets on such a queue to prevent using all pbufs for loopback communication (could be turned off)

Simon Goldschmidt <goldsimon>
Group administrator
Tue 15 Jan 2008 07:35:30 PM UTC, comment #8: 

Luca,
I must say I more and more favour your solution. netif.c seems like the right place to put code like this.

However, the one big disadvantage I see (and which is the reason for preferring the loopif.c after all) is including different code for multithreading and non-OS setups in netif.c. Somehow I think this code is rather unclean (although I introduced it in loopif.c, hehe).

But in the end I think there is no other solution than having the MULTITHREADING define and it doesn't really matter where that code resides...

The reason for preferring one big queue would be that it is easier to handle, i.e. the user/porter doesn't have to call netif_poll for each netif but only once...

Jonathan,
I don't know if the future-compatble patch helps much: users would be tempted to call netif_poll() with their netif, which would lead strange results with 1.3.0 since that function would contain code that only works on loopif-netifs (netif->state == struct loopif_private).

Simon Goldschmidt <goldsimon>
Group administrator
Tue 15 Jan 2008 10:16:11 AM UTC, comment #7: 


> [Jonathan] I'm a bit unconvinced of some of the naming

I'm not proud of them, my muse was on vacation that day.

Some ideas to improve naming and the rest fo my patch.
- IP_TO_SELF >> LWIP_NETIF_LOOBPACK
- IP_TO_SELF_MULTITHREADING >> LWIP_NETIF_LOOBPACK_MULTITHREADING
  (argh, very long)
- netif.self_{first|last} >> netif.loop_{first|last}
- loopif_poll() >> no idea (Simon?)
- LWIP_LOOPIF_MULTITHREADING: will it be needed anymore, in case
  the loop behaviour is moved into netif?
- How about setting the default value of IP_TO_SELF_MULTITHREADING
  to (!NO_SYS)?
- Moving the _output() function from loopif.c to netif.c, I changed
  from pbuf_alloc(PBUF_RAW, ...) to pbuf_alloc(PBUF_LINK, ...). I need
  an ack/nack on that by someone with more knowledge of pbufs.

Comments?

> [Simon] I'd still be more pleased to leave the code in loopif.c

I see no advantage in that apart from backward compatibility (which is a no-issue, according to Jonathan).
After all you would introduce new code, part of which tricky (the fourth of your points is a workaround to make a single loopif mimic the proper behaviour of many real netifs). My patch is just moving good code to a place where it does better.

What's the pros of having one (big) queue, compared to many (smaller) ones?

Luca Ceresoli <lucaceresoli>
Mon 14 Jan 2008 06:54:59 PM UTC, comment #6: 

I'm think the function 'netif_poll' could be misleading, since other netifs can be in polling mode also, which means they simply don't use interrupts...

Aside from that, I'd still be more pleased to leave the code in loopif.c (only an idea):

- Create a function 'netif_output(struct netif, struct pbuf, struct ipaddr)' that decides whether to call netif->output or loopif->output.
- loopif->output could then save the netif and use that to call that netif's->input.
- That way, we would only have one queue (in loopif.c) and very little change to the existing code.
- The only big change would be to propagate the loopif netif and to include netif in the loopif linked list (which could be a bit tricky given the current pbuf list).

Simon Goldschmidt <goldsimon>
Group administrator
Mon 14 Jan 2008 04:27:03 PM UTC, comment #5: 

This seems much better, thanks. I'm a bit unconvinced of some of the naming ("ENABLE_SELF" etc.) and think just referring to it as loopback packets may be clearer, but that's a detail to deal with later.

I'd like to avoid introducing backward-compatibility issues - especially since loopif_poll() is a function we would only be introducing for the first time in 1.3.0, so it would seem a shame to do so knowing we're likely to be obsoleting it immediately.

So, in advance of 1.3.0 may I suggest a future-compatibility patch. Even if there are question marks over the detail of Luca's change, if we can agree on the principle of there being a netif_poll() function in future for the NO_SYS case, then applying this before 1.3.0 will save a bit of hassle (more for our users than for us!).

Ok?


(file #14795)

Jonathan Larmour <jifl>
Group Member
Fri 11 Jan 2008 06:21:03 PM UTC, comment #4: 


> Is having the source IP address set to 127.0.0.1 even a big issue?

Not necessarily, but the first patch led to dropped packets at the UDP level when the pcb is bound to a local address, and maybe other problems also involving TCP. Going on with that patch would have led to patching udp.c and perhaps tcp.c and icmp.c. Sounds bad for IP-related things. This was the meaning of "invasive", not the quantity of code.
Apart for this, ain't it conceptually wrong to send through the loopif a packet directed to, say, 192.168.0.1?

My next try is basically the same as the previous, plus the removal of most code from loopif. The loopif has now its behaviour for free, being a netif.
Apart from coding style and where-to-place-it, this is substantially what was suggested so far (comments #1, #2 and #3): one piece of code to be used for both loopif and IP_TO_SELF.

The only real difference is that the IP_TO_SELF feature is available without creating a loopif.
More logical implementation and user interface, that's it.

In this new patch there's still some pre-processor dirt to maintain compatibility with the current loopif interface: loopif_poll(), the compile-time options and most notably the multithreading defines.
If it's worth breaking compatibility to clean this code, I can't say.


(file #14784)

Luca Ceresoli <lucaceresoli>
Fri 11 Jan 2008 04:44:08 PM UTC, comment #3: 

I too am sceptical that this is a good general solution.  It's definitely not something for 1.3.0.

If we want this functionality we'd have to integrate it better to avoid the code duplication, but I'm not sure I understand why loopif and appropriate routing rules doesn't work as is.

Kieran Mansley <kieranm>
Group Member
Fri 11 Jan 2008 03:02:08 PM UTC, comment #2: 

After taking the approach of changing ip_route() there were some issues, but you said these would be too invasive to fix. But I wouldn't have thought it would be that invasive; or at least less so than the patch here. It seems not very good to have so much new code here, especially since it's similar to loopif.

Is having the source IP address set to 127.0.0.1 even a big issue?

Or even if ip_route() doesn't work so well I would have thought something like this in ip_output_if() would do (similar to what you were already proposing changing:

#if LWIP_HAVE_LOOPIF
  if (ip_addr_cmp(dest, &netif->ip_addr)) {

    /* Packet to self, enqueue it */

    LWIP_DEBUGF(IP_DEBUG, ("Passing to loopif"));



    return loopif_output(netif, p, dest);

  } else

#endif /* LWIP_HAVE_LOOPIF */

  {
    LWIP_DEBUGF(IP_DEBUG, ("netif->output()"));



    return netif->output(netif, p, dest);

  }

And make loopif_output no longer be static.

Jonathan Larmour <jifl>
Group Member
Fri 11 Jan 2008 02:20:21 PM UTC, comment #1: 

First: I'd like to see this general feature to be included in the stack.

But to get this included into the core, it would have to be merged somehow with loopif: otherwise, we have nearly the same code twice.

And I'm not sure whether it isn't too late for 1.3.0 :-(

Simon Goldschmidt <goldsimon>
Group administrator
Fri 11 Jan 2008 02:04:06 PM UTC, original submission:  

Support sending packets with a destination IP address equal to the netif IP address, looping them back up the stack (off by default).

See previous discussion here:
http://lists.gnu.org/archive/html/lwip-devel/2008-01/msg00057.html

Both implementation and usage are quite the same of loopif.c.
To make it work, define IP_TO_SELF=1 in lwipopts.h. If NO_SYS==1, also call netif_poll() in the mail loop or from a timer.

Luca Ceresoli <lucaceresoli>

 

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

Attached Files
file #15851:  netif_input.patch added by lucaceresoli (812B - text/x-patch)
file #14825:  self-netif3.patch added by lucaceresoli (15KiB - text/x-patch)
file #14795:  netif.poll.future.compat.patch added by jifl (4KiB - text/x-patch - Future compatibility patch for 1.3.0)
file #14784:  self-netif2.patch added by lucaceresoli (14KiB - application/octet-stream)
file #14783:  self-netif.patch added by lucaceresoli (8KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by lucaceresoli (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 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-06-19 goldsimon StatusReady For Test Done
        Open/ClosedOpen Closed
    2008-06-17 goldsimon StatusIn Progress Ready For Test
    2008-06-16 goldsimon StatusReady For Test In Progress
    2008-06-16 lucaceresoli Attached File- Added netif_input.patch, #15851
    2008-06-12 goldsimon StatusNone Ready For Test
        Assigned toNone goldsimon
    2008-01-18 lucaceresoli Attached File- Added self-netif3.patch, #14825
    2008-01-14 jifl Attached File- Added netif.poll.future.compat.patch, #14795
    2008-01-11 lucaceresoli Attached File- Added self-netif2.patch, #14784
    2008-01-11 lucaceresoli Attached File- Added self-netif.patch, #14783

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code