lwip-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [lwip-devel] [bug #27034] Invalid ASSERT in pbuf_alloc()


From: Alain Mouette
Subject: Re: [lwip-devel] [bug #27034] Invalid ASSERT in pbuf_alloc()
Date: Wed, 15 Jul 2009 18:58:36 -0300
User-agent: Thunderbird 2.0.0.17 (X11/20080914)

Hi,

am getting woried about this thread. PPP is working and is important to many people...

PLEASE, don't break that. As was said, it not very well mantained, so if it stops working, it will be *chaos*

I would like to remind that *doing something useless is not a bug*. A bug is when the program does not behave as expected or crashes.

Me, for one, I only opted for lwip because it has ppp. If it stops working I will be stuck in an old verion forever. Ppp is too complex to mantain but it is good quality code and works fine (it was copied from bsd or similar).

Please...
Alain

Bill Auerbach escreveu:
 pbuf_alloc(PBUF_RAW, 0, PBUF_POOL);
The above is used in PPP,
To me, that's not a reason to allow zero-length pbufs

I was only pointing out if you assert for it this will come up and cause more 
work fixing.

the PPP code is
unfortunately not very actively maintained and thus may (beside this
one) contain other bugs or invalid code: lines like

     *((u_char*)nb->payload + nb->len++) = PPP_ESCAPE;

Ppp may not be the best example code, but if it works at least it's usable to 
some.  Now that you said this, how will you explain that invalid or buggy code 
(or illegal use of internal data) is used in production code in CVS? :-)

I'd rather fix that than allowing
zero-length pbufs throughout the stack (which is in most cases a bug).

Or expand the API to access these items.  Payload is one that I presume should 
be off limits too, but isn't accessing it essential to zero-copy support and it 
must be used in low-level drivers.  There are problems reported from accessing 
len instead of tot_len.  An API for them, even just hidden in macros, would 
keep it efficient and lower misuse.  E.g.

#define pbuf_get_payload(p) (p->payload)

So you're fiddling around with pbuf->len, too? Code like that is not
supported!

It won't require support and I'll fix it if updates break it.  Being able to 
fiddle with this stuff allowed me to achieve what I wanted without (in this 
case) changing any base lwIP code.

In general, application code playing around with lwIP struct
members without using API functions provided for that use is (except for
some rare cases) not allowed/supported.

My case is rare then. :-)  TCP is inefficient and not real-time on most 
sub-200MHz embedded processors.  UDP is fast but not reliable.  So I wrote a 
protocol for UDP that is reliable but I wanted to build it on pbufs since that 
is the low-level interface to sending packets.  My program, of which the 
high-speed required part was TCP now uses this protocol with only 2 or 3 code 
line changes.

I've learned a lot about lwIP and where it spends a lot of time.  If you want 
to create a task to make lwIP 3-4 times faster, I can help you do this. 
Efficiency isn't prioritized very high and I had to have it, so I resorted to 
doing what I had to.  Sorry I fiddled with the internals - it saved this 
project.

Unfortunately, with C, it can't
really be avoided by our code/files in a decent way.

You can make all the members you want untouched const.  Yes, the internal code has to be 
changed to a cast to change them, but that's one time at the benefit of forcing people to 
do things properly.  And people like me who have a need otherwise can cast in their code 
to make the point "I broke the rules".  Please don't try to limit what we *can* 
do because it's not what we're *supposed* to do.

[There are some
rare cases like netif->flags= where most available code uses the
internal struct members, but I'd vote for introducing API
functions/defines for these places, too.]

I agree.  I added some functions for hostname support.  API functions can be 
macros (which lwIP already utilized a good bit) and efficiency doesn't have to 
be sacrificed.  You could use inline functions but not all compilers handle 
that the same, if at all.

You should know the length before calling pbuf_alloc() and pass it
instead of 0.

I'll explain why I did this: I have a static pbuf that has the EHTHDR, IPHDR, and 
UDPHDR filled in and those static parts checksummed.  I chain this to a payload 
pbuf.  When I need to send UDP data, I update the 3 or 4 items in the IPHDR and 
UDPHDR, and add the changes to the static checksum and fill it in.  I update the 
payload pointer, len and tot_len of each and call the netif->linkoutput.  It's 
simple and I can stream at 970MbS (my device can't support this but it's what I 
get if I just point to memory and send data).  One of the reasons for the speed is 
the repeated pbuf_alloc/pbuf_free/udp_sendto calls took 2/3rds of the overall 
time. I'm also able to maintain a TCP connection for control packets and 
connection processing.  It's worked great for me.

Bill



_______________________________________________
lwip-devel mailing list
address@hidden
http://lists.nongnu.org/mailman/listinfo/lwip-devel






reply via email to

[Prev in Thread] Current Thread [Next in Thread]