[bug #2595] loopif results in NULL reference for incoming TCP packets

classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[bug #2595] loopif results in NULL reference for incoming TCP packets

Ashley Duncan

Follow-up Comment #4, bug #2595 (project lwip):

This is still open (for a while now...) and I must say I don't like the
solution as it introduces 1 ms delay for each packet going over the loopif.

I think there are 2 possibilities solving this:

1. If multithreaded stack is used, netif->input() should be tcpip_input() for
the loopif. With that, we can call netif->input directly in loopif_output()
since the pbuf is put in a queue.

2. If callback environment is used, we need a simple linked list for incoming
packets where we put the pbuf. That queue must then be processed in the main
loop, through something like 'poll_driver(netif)' (see
http://www.sics.se/~adam/lwip/os.html).

Some other remarks:
- memcpy() into a new pbuf is of course still needed
- using PBUF_POOL would make more sense since this is the type which should
be used for input
- the current struct that is malloc'ed using mem_malloc() and passes the
netif pointer to loopif_input() is not needed since there can be only _one_
interface with 127.0.0.1 -> the loopif netif struct can be static!

The only thing we would have to know is if we are running in multithreaded or
polling mode.

-> Are there any other 'modes' this wouldn't work with?
Hoping for your comments...

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?2595>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #2595] loopif results in NULL reference for incoming TCP packets

Ashley Duncan

Follow-up Comment #5, bug #2595 (project lwip):

You could test for ETHARP_TCPIP_INPUT - that's what controls the presence of
tcpip_input.


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?2595>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #2595] loopif results in NULL reference for incoming TCP packets

Ashley Duncan

Follow-up Comment #6, bug #2595 (project lwip):

>You could test for ETHARP_TCPIP_INPUT - that's what controls the presence of
tcpip_input.

Unfortunately, I think I can't: first it could be either ETHARP_TCPIP_INPUT
or ETHARP_TCPIP_ETHINPUT, second, this does not make sure the user is really
using the tcpip_thread and not using the polling mode and simply not
deactivating the above defines.

I think NO_SYS could be more like it (because it does not make sense to use
tcpip_thread() if sys_mbox_fetch() is a null-define...
But still, it is not explicitly documented (I think), that NO_SYS=1 means
raw-API/polling only and NO_SYS=0 means netconn/socket-API & tcpip_thread().

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?2595>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #2595] loopif results in NULL reference for incoming TCP packets

Ashley Duncan

Follow-up Comment #7, bug #2595 (project lwip):

Just, I also note some remarks in "local connection failed on loopif, race?"
(forum's thread March 30th 2007):

If you got a mem_malloc( sizeof( void *[2]))c error, the "r" pbuf is not
freed (and "one day" you will got a "not enough memory") Inside sys_timeout,
if you got a memp_malloc(MEMP_SYS_TIMEOUT) error, there is no way to know by
the caller is the timer is initialized (and to free any resources in this
case, and so "one day", you will got a "not enough memory").
Last, using a timer to communicate pbuf between two "contexts" (and in fact,
this is the same, tcpip_thread) seems a little strange...

So, things to do seems to be :

- add pbuf_free when "if( NULL == arg ) {" is true
- add a return type de sys_timeout, and check result to do a pbuf_free and a
mem_free.
- prehaps redesign with a mbox + a new thread (but, I don't think it's a good
idea)?


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?2595>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #2595] loopif results in NULL reference for incoming TCP packets

Ashley Duncan

Follow-up Comment #8, bug #2595 (project lwip):

I think making a call to tcpip_callback() with a suitable callback function
and argument would solve a lot of these problems in one fell swoop!


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?2595>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #2595] loopif results in NULL reference for incoming TCP packets

Ashley Duncan

Follow-up Comment #9, bug #2595 (project lwip):

>prehaps redesign with a mbox + a new thread (but, I don't think it's a good
idea)?

In fact, Simon idea in multithread environment is better...

>But still, it is not explicitly documented (I think), that NO_SYS=1 means
raw-API/polling only and NO_SYS=0 means netconn/socket-API & tcpip_thread().

But loopif.c can used sys.c functions (sys_timeout) in a raw-api interface,
right? So, meaning of NO_SYS you propose seems not right?


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?2595>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #2595] loopif results in NULL reference for incoming TCP packets

Ashley Duncan

Follow-up Comment #10, bug #2595 (project lwip):

About comment #8, tcpip_callback doesn't seems to be used, except for PPP,
so, associated code would have to be disable, like others tcpip_ functions,
to reduce footprint, and only enable if necessary.

Next, if we suppose - and I don't think it's true for all lwIP's users - that
loopif can only be use with tcpip_thread, so, perhaps the good thing to do is
just to call tcpip_input or tcpip_ethinput (with a pbuf recopy), like Simon
suggest in comment #4.

About this last comment, is someone use NO_SYS=0 without tcpip_thread?


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?2595>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #2595] loopif results in NULL reference for incoming TCP packets

Ashley Duncan

Follow-up Comment #11, bug #2595 (project lwip):

I think it is possible for someone to use NO_SYS=0 without tcpip_thread. Just
because someone is using the raw API doesn't mean it is a single-threaded
system - it may need to interact with other threads.

However, clearly the user could implement that with their own OS native
functions rather than the lwip sys API. The question is whether we want to
force them to have to do so, even if they might find the sys API convenient.
The answer can be yes, but doesn't have to be. Personally I think yes, and if
so, we need to document it.



    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?2595>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #2595] loopif results in NULL reference for incoming TCP packets

Ashley Duncan

Follow-up Comment #12, bug #2595 (project lwip):

>The question is whether we want to force them to have to do so, even if they
might find the sys API convenient. The answer can be yes, but doesn't have to
be. Personally I think yes, and if so, we need to document it.

I'm not sure to understand you, my english isn't good enough :) : if you want
to say that "users don't have to use lwIP internal features like sys functions
or mem functions", yes, I'm agree with you. More, if we can't change some
internal features because some developers use them directly, it will be
difficult to modify or improve lwIP...


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?2595>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #2595] loopif results in NULL reference for incoming TCP packets

Ashley Duncan

Follow-up Comment #13, bug #2595 (project lwip):

Sorry, I'll phrase it differently as I see now it isn't easy English for
non-native English speakers:

In the situation I described, some developers will find using the sys API
convenient. We can consider forcing them to have to use their own OS native
functions instead. I think that would be a good idea, so NO_SYS=0 must mean
there is a tcpip_thread. But if so, we need to document this.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?2595>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #2595] loopif results in NULL reference for incoming TCP packets

Ashley Duncan

Follow-up Comment #14, bug #2595 (project lwip):

Funny ;-)

Whatever, I think directly calling tcpip_callback() or somethink is not a
nice solution. Loopif is a 'normal' netif and has its input function pointer.
The only question is when to call it (better: whether to call it or leave that
to a polling function).

Maybe we should make it an option for loopif only since we don't seem to know
any option until now which tells us in which way the stack is run.

This could also directly includ a function like loopif_poll() (though maybe
with a different name?) which has to be called in the main loop when not
using the tcpip_thread.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?2595>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #2595] loopif results in NULL reference for incoming TCP packets

Ashley Duncan

Follow-up Comment #15, bug #2595 (project lwip):

Yes, you're right about netif and its input function pointer. Note there is
always will have to be remove I think.

#if 0 /** TODO: I think this should be enabled, or not? Leon */

Contrib modules initialize loopif like this :

#if LWIP_HAVE_LOOPIF  
  IP4_ADDR(&gw, 127,0,0,1);
  IP4_ADDR(&ipaddr, 127,0,0,1);
  IP4_ADDR(&netmask, 255,0,0,0);
 
  netif_set_default(netif_add(&loopif, &ipaddr, &netmask, &gw, NULL,
loopif_init, tcpip_input));
#endif

if it possible/normal to use a loopif with other IP values than these ones?
If not, it would be good to declare const in loopif.h/.c.

loopif_poll() is a good idea and a good name.

I also think we have to clarify some define like NO_SYS,
SYS_LIGHTWEIGHT_PROT... and to have a explicit define for declare we use
tcpip.h/.c (LWIP_MULTITHREAD?), and another one to disable/enable
tcpip_callback and associated code is not necessary.

LWIP_MULTITHREAD (or LWIP_SEQUENTIAL_API, or other) will also help to not
build tcpip.c, api_msg, api_lib, sockets, if not define (I suppose we can add
a #error if these files are build with LWIP_MULTITHREAD=0).

If we change names for NO_SYS & SYS_LIGHTWEIGHT_PROT, we also can add #error
to inform and force users about changes...






    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?2595>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #2595] loopif results in NULL reference for incoming TCP packets

Ashley Duncan

Follow-up Comment #16, bug #2595 (project lwip):

Strictly the loopback interface could have other loopback addresses on it, as
long as they are within the 127/8 address block. I've seen this done on other
systems althugh the applications tend to be obscure. So it is possible,
people sometimes use it, but it's not normal by any means.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?2595>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #2595] loopif results in NULL reference for incoming TCP packets

Ashley Duncan

Update of bug #2595 (project lwip):

                  Status:                    None => In Progress            
             Assigned to:                    None => goldsimon              

    _______________________________________________________

Follow-up Comment #17:

>Strictly the loopback interface could have other loopback addresses on it,
as long as they are within the 127/8 address block.

OK, so no static configuration.

I'll work on a patch and take NO_SYS for the decision to input or not, for
the moment. We'll have to decide whether this is OK or not.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?2595>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #2595] loopif results in NULL reference for incoming TCP packets

Ashley Duncan

Follow-up Comment #18, bug #2595 (project lwip):

Sorry for the interruption.  I'm the original poster of the thread "local
connection failed on loopif, race?" as mentioned in comment#7.  I have done a
couple of tests with loopif plus RAW API based client/server on two different
platforms(FreeRTOS and unixsim running on FreeBSD).

It's interesting to note that with the current sys_timeout() based
implementation, the client hung after sending the first string after
connected to the server(for debugging traces, see
http://lists.gnu.org/archive/html/lwip-users/2007-03/msg00131.html ).

The attached patches(loopif-q[2].patch) are the thread/mbox based
implementation(with slightly modifications) which passed the test described
in aforementioned environment. Since slipif, pppif or even ethernetif in many
cases would have its own thread to process I/O requests, I think it's
reasonable for loopif to put the packet I/O to a different thread context.
The downside is that the patch didn't try to address NO_SYS, yet.

In addition to that, it looks to me that the loopback can be something other
than 127/8 as there doesn't appear to be any restriction on this inside the
stack.

(file #12510, file #12511)
    _______________________________________________________

Additional Item Attachment:

File name: loopif-q.patch                 Size:3 KB
File name: loopif-q2.patch                Size:3 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?2595>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #2595] loopif results in NULL reference for incoming TCP packets

Ashley Duncan

Follow-up Comment #19, bug #2595 (project lwip):

RE comment #18:

The two patches (file #12510/12511) will work, but creating an own thread is
not really necessary. It is easier to create a simple linked list of pbufs
and call something like loopif_poll() in the main lwip loop (loopif_poll
would call netif->input() for every packet in the linked list.

That way, you save yourself from having an additional thread (which saves
context-switches and thus, time) and a linked list will be way faster than
sys_mbox_t. Also, it's compatible both with NO_SYS=1 and NO_SYS=0.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?2595>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #2595] loopif results in NULL reference for incoming TCP packets

Ashley Duncan

Follow-up Comment #20, bug #2595 (project lwip):

I have created a patch which should solve this problem. Note that to switch
between multithreading (call netif->input directly as it puts the packet into
an mbox) and raw-API only (put the packet into a linked list, let
loopif_poll() fetch it), you have to define LOOPIF_MULTITHREADING to 1 or 0
(since we don't yet have an lwIP-wide switch to know this).

*** Note that it does compile, but I did not (yet) have the time to test it.

Any comments are welcome, as I really would like to have a working loopif ;-)

(file #12512)
    _______________________________________________________

Additional Item Attachment:

File name: loopif_linkedlist.patch        Size:5 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?2595>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #2595] loopif results in NULL reference for incoming TCP packets

Ashley Duncan

Follow-up Comment #21, bug #2595 (project lwip):

Ok, I've tested the patch a little and although my application (for which I
need the loopif for) does not work, the loopif itself works fine ;-)

Should I check this in or does anyone dislike the extra-configuration option
in loopif.h (LOOPIF_MULTITHREADING)?

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?2595>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #2595] loopif results in NULL reference for incoming TCP packets

Ashley Duncan

Follow-up Comment #22, bug #2595 (project lwip):

Reply to comment#20.  The patch looks okay to me. Here are a few compilation
issues I observed whilst building with Paradigm C++ Pro:
1. Need to include <lwip/sys.h> to have
SYS_ARCH_DECL_PROTECT()/SYS_ARCH_{UN}PROTECT() support;
2. There's a SYS_ARCH_DECL_PROTECT() in loopif_init() but no correlated
SYS_ARCH_{UN}PROTECT().

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?2595>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[bug #2595] loopif results in NULL reference for incoming TCP packets

Ashley Duncan

Follow-up Comment #23, bug #2595 (project lwip):

comment #22, supplemental:
3. "LWIP_UNUSED_ARG(ipaddr);" in loopif_output().

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/bugs/?2595>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
12