[lwip-devel] [task #6933] Review usage of ASSERT and error handling with LWIP_NOASSERT

[Frédéric Bernon]

Follow-up Comment #7, task #6933 (project lwip):

(It's a bit longer, sorry, but I hope to be clear about what I think, and my
english is not very good ;) )

Yes, I confirm, I think a production code for that kind of probelm should be
...no code !!! If a such thing happen and the application developer check it,
I think the problem you got is higher (memory corruption or else...).

In a such case, I prefer an exception handled by the processor and/or the
watchdog (in a such case, we could write a log, send a message on a serial
link, close a contact, etc...), than a "masked" problem where the action is
not done, and where the customer thing the product is not reliable (and have
to reset it manually). With the first strategy, we could get a field-feedback
and study the case to fix the problem, with the second one, you don't have any
idea of the problem.

If you think your developers can do such errors, you're right to check. But
because choose such or such strategy is "product dependant", I think we
should have an option to define the "protection level" that each one want for
such problems.

Resume of what we currently get for errors handling:

LWIP_DEBUG is a very useful macro for lwIP developers or ports developers to
search error at development time, to try to understand what do the stack,
etc.. We have possibility to set such or such error class, level, etc... It's
more a way to have informations about execution. But it don't have to stay in
a final product (slower execution). 

LWIP_ASSERT is also a very useful macro for lwIP developers or ports
developers macro at development time: if there is a programmer error, you can
catch it, a bug in the stack, you can catch it... But it don't have to stay in
a final product (slower execution).

LWIP_ERROR is defined, but not used in the stack (to remove?).

So, LWIP_ASSERT is too generic to my point of view. I think there is two kind
of errors to handle: "possible" and "fatal/bug".

For the first class of error ("possible"), it's true they have to be handled
by the stack: no enought memory (ERR_MEM, ERR_BUF...), connection reset or
aborted (ERR_RST, ERR_ABRT...). They are "normal" or "possibles" errors. Most
of time, they are due to the device's environment: lot of devices on the LAN,
the network doesn't filter packets, lot of packets lost, route failure, link
disconnect, DoS attacks.... For that, the production code have to contain
instructions to have a clean error handling.

For the second class of errors, you have to handle something impossible or we
should really never happen: this kind of errors should be recorded for field
feedback, but most of time, you have to reset your product. So, on the field,
you force a "hardware hot reset" by a processor instruction, or stop to feed
the watchdog and wait the reset.

So, "if (conn == NULL)" is in the second class, because, it you're yet at
development time, when you do your unit tests or validation tests, you will
catch a such error with the ASSERT and you fix it before release. So, just an
ASSERT is enough. If you not sure about your application level, or if you
don't have time to check, ok, you can try to "check and return", but you just
mask the problem: what will you do on field? And for any teams spending time
to tests their product, you have a redundant an not useful code.

Last, such test could be consider like no enought: when you receive an
address to do a write or a recv, do you check it is on the valid memory
space? I don't think. However, such test is at the same level than checking
"conn".

So, I propose this kind of solution:

In debug.h, we add

#ifndef LWIP_FATAL
#define LWIP_FATAL(m,f,h) if (f) { LWIP_PLATFORM_ASSERT(m); h;};
/* mESSAGE, fATAL_ERROR, hANDLER */
#endif

In the lwIP code:

LWIP_FATAL("netconn_close: invalid conn", (conn==NULL), {return RR_ARG;});

So, like this, we, the lwIP developers do a basic fatal error handling
(continue is "fatal" for the stack).

But, if you're sure about your application developers, you could add in your
cc.h file something like that (it will replace the debug.h one):

#define LWIP_FATAL(m,f,h) if (f) { LOG_ERROR_IN_FLASH(m); /*and wait the
crash to force reset*/};

More, if you're sure about your developers, your hard, etc.. you could simply
say "I will never get such problem" and write :

#define LWIP_FATAL(m,f,h) {;}

Like this, this kind of error is handled by each lwIP developers, using his
own report "tools" for field feedback.

Comments ?



    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/task/?6933>

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