tasklwIP - A Lightweight TCP/IP stack - Tasks: task #6933, Review usage of ASSERT and error...

 
 

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

task #6933: Review usage of ASSERT and error handling with LWIP_NOASSERT

Submitter:  Simon Goldschmidt <goldsimon>
Submitted:  Thu 24 May 2007 07:22:12 AM UTC
   
 
Category:  None Should Start On:  Thu 24 May 2007 12:00:00 AM UTC
Should be Finished on:  Thu 24 May 2007 12:00:00 AM UTC Priority:  1 - Later
Status:  Done Privacy:  Public
Assigned to:  goldsimon Percent Complete:  100%
Open/Closed:  Closed Planned Release:  None
Effort:  0.00

Jump to the original submission

Mon 02 Jul 2007 07:19:50 PM UTC, comment #23: 

Ooops, this is still set to 0% :-)

I think I've checked pretty much all asserts (everything but ppp) and converted some of them to LWIP_ERROR.

I regard this as done.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 22 Jun 2007 08:07:49 PM UTC, comment #22: 

OK, I'll start with that now.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 21 Jun 2007 09:15:59 PM UTC, comment #21: 

Finally, you don't want to chnage LWIP_ERROR semantics now?

Frédéric Bernon <fbernon>
Group Member
Thu 21 Jun 2007 07:43:05 PM UTC, comment #20: 

I think this is done and can be closed. Does anyone think different?

Simon Goldschmidt <goldsimon>
Group administrator
Mon 18 Jun 2007 04:07:08 PM UTC, comment #19: 


> in this case, the assert test isn't useful: you already know that (p == NULL). Isn't it better to directly call LWIP_PLATFORM_ASSERT


I'd prefer calling LWIP_ASSERT("...", 0) instead of directly calling LWIP_PLATFORM_ASSERT, since that isn't undefined with LWIP_NOASSERT!

> why do you have move the assert inside the "if (p == NULL)"
> (it was just before in previous code)? After all, it does
> the same job...


Exactly. And because of that, I wanted to have only one if-statement to make the code better to read...

> I would like to know why LWIP_ASSERT, LWIP_ERROR and
> LWIP_DEBUGF do the job with a "do {...} while(0)"?
> Why this do/while?


That's because the do/while is the most portable way to make a function-like-define work in most places, I think. Don't ask me for examples, though!

Simon Goldschmidt <goldsimon>
Group administrator
Mon 18 Jun 2007 07:48:58 AM UTC, comment #18: 

A question about some asserts I seen. Take this sample from pbuf_free:

  if (p == NULL) {
    LWIP_ASSERT("p != NULL", p != NULL);
    /* if assertions are disabled, proceed with debug output */
    LWIP_DEBUGF(PBUF_DEBUG | LWIP_DBG_TRACE | 2, ("pbuf_free(p == NULL) was called.\n"));
    return 0;
  }

1/ in this case, the assert test isn't useful: you already know that (p == NULL). Isn't it better to directly call LWIP_PLATFORM_ASSERT?

2/ Just to understand, why do you have move the assert inside the "if (p == NULL)" (it was just before in previous code)? After all, it does the same job...

3/ I would like to know why LWIP_ASSERT, LWIP_ERROR and LWIP_DEBUGF do the job with a "do {...} while(0)" ? Why this do/while?



Frédéric Bernon <fbernon>
Group Member
Sun 17 Jun 2007 07:10:49 PM UTC, comment #17: 

I think I've finished... didn't change LWIP_ERROR semantics yet.

Simon Goldschmidt <goldsimon>
Group administrator
Sun 17 Jun 2007 03:33:17 PM UTC, comment #16: 

Ok, post a comment when you think you have terminate, I will check your change to be sure all is ok if you want...

Frédéric Bernon <fbernon>
Group Member
Sun 17 Jun 2007 02:53:11 PM UTC, comment #15: 

Maybe we should reaqlly change LWIP_ERROR to LWIP_ASSERT semantics now, as LWIP_PLATFORM_ASSERT is called, which prints something like "Assertion %s has failed" and that is very misleading for the current LWIP_ERROR...

Simon Goldschmidt <goldsimon>
Group administrator
Sun 17 Jun 2007 02:50:44 PM UTC, comment #14: 


> Last, just to be sure, we are agree to say that all LWIP_ASSERT don't have to be replaced by LWIP_ERROR, right?


Yes. But I think there are many places where the replacement is needed in order to safely set LWIP_NOASSERT to 1!

I'm currently looking through the stack and changing some files. Maybe dhcp.c has a little much error checks... but for the rest, I feel it's needed.

I'm not changing LWIP_ERROR for now since I already have made the changes for the current define...

Simon Goldschmidt <goldsimon>
Group administrator
Sun 17 Jun 2007 02:39:47 PM UTC, comment #13: 


>That way, one can easily change from ASSERT to ERROR without fearing to forget to change the expression to the negative.


I feel better to say that "e" is an "error expression", and it avoid a "not" instruction. But if you really prefer, change it. We can even say it's more coherent to have the same semantics for ASSERT and ERROR. Of course, don't forget to upgrade existing LWIP_ERROR.

Last, just to be sure, we are agree to say that all LWIP_ASSERT don't have to be replaced by LWIP_ERROR, right?

Frédéric Bernon <fbernon>
Group Member
Sun 17 Jun 2007 02:29:25 PM UTC, comment #12: 

Frédéric, is it OK for you to change

#define LWIP_ERROR(m,e,h) do { if (e) {...

to

#define LWIP_ERROR(m,e,h) do { if (!(e)) {...

?
That way, one can easily change from ASSERT to ERROR without fearing to forget to change the expression to the negative.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 13 Jun 2007 04:03:35 PM UTC, comment #11: 


>I thought you wanted to integrate the 'return X;' into the definition of LWIP_FATAL, must have misunderstood that...


No, the "h" parameter for LWIP_ERROR is to define a good action in the function context when we got error, but it's not always the same action: sometimes we have to return a err_t (like for netconn_send), sometimes a netbuf* (like for netconn_recv)...

So, in debug.h, we will add:

#ifndef LWIP_ERROR
#define LWIP_ERROR(m,e,h) if (e) { LWIP_PLATFORM_ASSERT(m); h;};
/* mESSAGE, eRROR, hANDLER */
#endif

In api_lib.c, in netconn_close, we will replace the current ASSERT by :

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

I update this part if you want (or you do it, like you want...).

Frédéric Bernon <fbernon>
Group Member
Wed 13 Jun 2007 01:17:35 PM UTC, comment #10: 


> Sorry, I don't really see (except the name) what is the difference between this...


I thought you wanted to integrate the 'return X;' into the definition of LWIP_FATAL, must have misunderstood that...

I'm OK with using LWIP_ERROR for that, the name would match the functionality!

Simon Goldschmidt <goldsimon>
Group administrator
Wed 13 Jun 2007 01:03:27 PM UTC, comment #9: 


>Because of that, I wouldn't call it LWIP_FATAL. Giving a wrong argument isn't fatal.


What is "fatal" is to continue with a such argument.

>Still I'm OK with changing the if(..) return ..; to something else. I don't think that including 'return ERR_ARG;' in LWIP_FATAL() is a good solution, though. It might be better to call id LWIP_CHECK_ARG() and give 'return X;' as another argument to that define...


Sorry, I don't really see (except the name) what is the difference between this...

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

...and this...

LWIP_CHECK_ARG(conn != NULL, return ERR_ARG);

Except the message with is important to give an information, and the "!=" (it avoid to add the "!" in the evaluation).

Last about the name, LWIP_CHECK_ARG is not so good (you could do that on an expression, not only an argument). I propose you to use LWIP_ERROR (which is currently not used, so, we can change it)...

Frédéric Bernon <fbernon>
Group Member
Wed 13 Jun 2007 10:46:21 AM UTC, comment #8: 

While I can understand Frédéric's need for a faster stack (and leaving away 'double' checks), I still think it is necessary to have some argument checks. Because of that, I wouldn't call it LWIP_FATAL. Giving a wrong argument isn't fatal.

Still I'm OK with changing the if(..) return ..; to something else. I don't think that including 'return ERR_ARG;' in LWIP_FATAL() is a good solution, though. It might be better to call id LWIP_CHECK_ARG() and give 'return X;' as another argument to that define...

e.g. LWIP_CHECK_ARG(conn != NULL, return ERR_ARG);

Simon Goldschmidt <goldsimon>
Group administrator
Tue 12 Jun 2007 05:47:56 PM UTC, comment #7: 

(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 ?


Frédéric Bernon <fbernon>
Group Member
Tue 12 Jun 2007 03:41:53 PM UTC, comment #6: 

Just to make sure I understand what your suggesting Frederic, which bit of the following example do you think shouldn't be there?

  if (conn == NULL) {
    LWIP_ASSERT("some error message", (conn != NULL));
    return ERR_ARG;
  }

In production code it will just become:
  if (conn == NULL)
    return ERR_ARG;

In debug code it will be effectively just:
  LWIP_ASSERT("some error message", (conn != NULL));

I think you're saying we should just get rid of it altogether as no one should be calling with conn == NULL.  However, while that's true, this will make sure they're not, while making sure production code is more reliable in the event of something really strange happening.

Kieran Mansley <kieranm>
Group Member
Tue 12 Jun 2007 03:31:45 PM UTC, comment #5: 


>I like the look of Simon's patch. We get both assertions when there is a programmer error in a debug build, and robust error returns (rather than just carrying on blindly) if there is such an error in release code.


But for code where application programmers check function return codes, it just add redundant code, and a such code shouldn't stay in a production code. Perhaps a macro defined in cc.h (or lwipopts.h) can help to handle both cases (see comment #2).

Frédéric Bernon <fbernon>
Group Member
Tue 12 Jun 2007 03:21:34 PM UTC, comment #4: 

I like the look of Simon's patch.  We get both assertions when there is a programmer error in a debug build, and robust error returns (rather than just carrying on blindly) if there is such an error in release code.


Kieran Mansley <kieranm>
Group Member
Mon 11 Jun 2007 12:39:42 PM UTC, comment #3: 

The change was done due to this patch:

http://savannah.nongnu.org/patch/?5958

Frédéric Bernon <fbernon>
Group Member
Mon 11 Jun 2007 12:20:11 PM UTC, comment #2: 

For api_lib.c patch, I'm disagree with you: we have replace the "conn==NULL" check by assert, because it's most an application programmer task (always done when you have good developers in your team) and it's redundant here (more cycles & footprint).

If your really want to handle such cases inside your code, why not something like (defined or not, perhaps with a define in cc.h):

#define LWIP_ASSERT2(x,y,ret) do { if(!(y)) { LWIP_PLATFORM_ASSERT(x); return ret;} while(0)

And let each one decide to handle or not such checks in there production code

More, for the same reason we have do this change, I propose to replace in sockets.c any code like this... "if (!sock) return -1;" ...by an assert (once again, it's at the application to check that, like it check to pass an valid value to API).


Frédéric Bernon <fbernon>
Group Member
Mon 11 Jun 2007 10:55:25 AM UTC, comment #1: 

I've put together a patch of places I would return error values in addition to asserting to keep the stack working with LWIP_NOASSERT.

Looking forward to hearing your comments.

(file #13009)

Simon Goldschmidt <goldsimon>
Group administrator
Thu 24 May 2007 07:22:12 AM UTC, original submission:  

At least some part of the stack might not handle errors correctly if compiled with LWIP_NOASSERT.

We should review all the files for correct error handling so that running the stack with LWIP_NOASSERT (which is faster) is safe.

Simon Goldschmidt <goldsimon>
Group administrator

 

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

Attached Files
file #13009:  noassert.patch added by goldsimon (14KiB - 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 fbernon (Posted a comment)
  • -email is unavailable- added by goldsimon (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 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-07-02 goldsimon StatusNone Done
        Percent Complete0% 100%
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2007-06-11 goldsimon Attached File- Added noassert.patch, #13009

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code