tasklwIP - A Lightweight TCP/IP stack - Tasks: task #6969, Review usage of conn->err in...

 
 

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

task #6969: Review usage of conn->err in netconn layer

Submitter:  Simon Goldschmidt <goldsimon>
Submitted:  Fri 01 Jun 2007 05:22:37 PM UTC
   
 
Category:  None Should Start On:  Fri 01 Jun 2007 12:00:00 AM UTC
Should be Finished on:  Fri 01 Jun 2007 12:00:00 AM UTC Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  goldsimon Percent Complete:  70%
Open/Closed:  Closed Planned Release:  1.3.0
Effort:  0.00

Jump to the original submission

Tue 11 Dec 2007 07:42:25 PM UTC, comment #34: 

Fixed

Simon Goldschmidt <goldsimon>
Group administrator
Tue 04 Dec 2007 10:09:51 PM UTC, comment #33: 

It's ok for me. "Minor" errors don't dead a pcb.

So, you can close it if you want.

Frédéric Bernon <fbernon>
Group Member
Wed 28 Nov 2007 09:36:57 PM UTC, comment #32: 

If it's OK I think we can finally close this as done!

Simon Goldschmidt <goldsimon>
Group administrator
Tue 27 Nov 2007 10:23:19 PM UTC, comment #31: 

Seems good for me. But I will try to simulate some cases to be sure .

Frédéric Bernon <fbernon>
Group Member
Tue 27 Nov 2007 09:28:53 PM UTC, comment #30: 

It's checked in: ERR_MEM, ERR_BUF and ERR_RTE are non-fatal. Also updated the err_to_errno table in sockets.c

Simon Goldschmidt <goldsimon>
Group administrator
Tue 27 Nov 2007 07:40:30 AM UTC, comment #29: 

definitively should go in!

Simon Goldschmidt <goldsimon>
Group administrator
Mon 26 Nov 2007 09:19:03 PM UTC, comment #28: 

If there are concerns about the suggestion, then just drop it. It's not important enough to be worth spending time debating. Go with the way you've been planning as far as I'm concerned!

Jonathan Larmour <jifl>
Group Member
Mon 26 Nov 2007 08:36:12 PM UTC, comment #27: 

Just to be sure, can we change the "planned release" to 1.3.0 ?

Frédéric Bernon <fbernon>
Group Member
Mon 26 Nov 2007 08:35:35 PM UTC, comment #26: 


>What I might like is something like a non-fatal ERR_IF.


Adding some new error codes doesn't really have a high cost. But in your case ("my driver is if someone tries to send a packet where the pbuf chain is more than the driver can cope with"), what should be the application processing ?

>Maybe we should define what the difference between ERR_MEM and ERR_BUF is. I think returning ERR_BUF here might work


I think we should return ERR_MEM on mem_alloc error, and on sys_xxx_new errors (even if in some cases, it could be better to return a "fatal" error code). ERR_BUF is more for memp and pbuf allocation errors.

>And another problem would be that all old errors would be non-fatal (since bit6 = 0), so that would have to be turned around so that only new code can generate non-fatal errors.


I think the initial idea which a "less than" compare "is simpler" to understand and maintain. More, if a non-fatal code is -5 and a fatal is -69 (-5 | bit6=1), how do you process the err_to_errno_table ?


Frédéric Bernon <fbernon>
Group Member
Mon 26 Nov 2007 06:51:48 PM UTC, comment #25: 

On the one hand, you're right: marking the whole connection as dead because of one packet isn't good. But on the other hand, a TCP packet that is enqueued and can't be sent because it is too long will never be split up, so the whole connection is dead, anyway.

Maybe we should define what the difference between ERR_MEM and ERR_BUF is. I think returning ERR_BUF here might work...


Simon Goldschmidt <goldsimon>
Group administrator
Mon 26 Nov 2007 06:00:03 PM UTC, comment #24: 


> So, perhaps can you give your list about what is "fatal", and
> what is not?


Well, that was my point :-) ... sometimes the same sort of error may be fatal or not.

For example, memory errors would be non-fatal - fine. For example one I have in my driver is if someone tries to send a packet where the pbuf chain is more than the driver can cope with. That packet can never ever be sent as it is, but ERR_MEM doesn't seem right because retrying will never help. But at the same time returning ERR_IF marks the whole connection as dead (which is what happens in current code until this task is complete) which doesn't seem right either because other packets would work. What I might like is something like a non-fatal ERR_IF.
 
But I was only suggesting the bitmask as an idea, that's all. And to be honest, the problem above isn't such a big problem. You don't have to agree, in which case what you had in IRC seemed okay. I guess what really matters is consistency and making sure everyone knows what errors really mean.

> About the bitmask, the problem could be with err_to_errno_table
> in sockets.c. But we could mask it (bit 6) I think.


Yes, that's also what I was thinking of.


Jonathan Larmour <jifl>
Group Member
Mon 26 Nov 2007 05:23:50 PM UTC, comment #23: 

Our consensus was to only let ERR_MEM, ERR_BUF and ERR_RTE be minor errors. In addition to that, we wanted to add ERR_RETRY (although I'm not sure we really need this, in ip_output_if (my new 'patch' for bug #21643), we can return ERR_RTE also). All other errors would be fatal. This solution might not solve all kinds of errors in the really correct way, but it's way better than now.

Anyway, I can't think of a situation where out-of-memory is a fatal error in api_lib.c (but please convince me of the opposite :) ...

The general idea of masking is OK to me, but when generating the error (e.g. in the netif driver), you often can't tell if the upper layer (e.g. api_lib.c) sees this as fatal. And another problem would be that all old errors would be non-fatal (since bit6 = 0), so that would have to be turned around so that only new code can generate non-fatal errors.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 26 Nov 2007 04:35:44 PM UTC, comment #22: 

Define what could be "fatal" and what is not is not very easy. As you have see in the IRC chat, we have spend time to try to define which error codes should be. But, of course, it's mainly based on the lwIP core, and since each developers can use any code like he wants in his driver. This is mainly before the is no a "strict" definition of each code and when to return it. So, I think that once the fatal code would be defined, there will be some port breakages. But in fact, since any code different than ERR_OK already cause to block send/recv operations, I'm not sure this is a real problem.

So, perhaps can you give your list about what is "fatal", and what is not?

About the bitmask, the problem could be with err_to_errno_table in sockets.c. But we could mask it (bit 6) I think.

Frédéric Bernon <fbernon>
Group Member
Mon 26 Nov 2007 04:20:04 PM UTC, comment #21: 

I saw some discussion on IRC about this task. Two things occur to me:

1) in my drivers, I had already been using ERR_IF to indicate a fatal error in driver operation. For example, trying to send on an interface that was never successfully brought up; or sending a pbuf chain with more elements in the chain than the driver supports (for drivers that support scatter-gather DMA).

So it's true that drivers sometimes need to provide fatal errors. But it's also true that drivers may have transient errors, and there is no good code for that. Which brings me to....


2) There seemed to be a bit of to-ing and fro-ing about which errors should be fatal or not. The problem seems to be that in some contexts a failure can be fatal, and sometimes not, even if the reason is the same (e.g. out of memory).

May I suggest instead making the internal representation of the error in conn->err be interpreted as a bitmask. The values are already negative, so instead of the top bit, maybe bit 6 could indicate if a failure is minor or fatal.

Then ERR_IS_FATAL() just needs to test that bit. ORing (or not ORing) that bit will not be any more code because the compiler will make it a constant either way.

Just a thought.

Jonathan Larmour <jifl>
Group Member
Mon 12 Nov 2007 11:02:03 PM UTC, comment #20: 

I don't see the problem with netconn->callback, can you give me for details about that? About comment #18, thoughts?

Frédéric Bernon <fbernon>
Group Member
Mon 12 Nov 2007 08:12:23 PM UTC, comment #19: 

Can this really wait until after 1.3.0? I think this (and maybe netconn->callback not being locked - what was the status of that?) is one of the last regions where lwIP isn't thread-safe (at least once bug #20900 is fixed)!

Simon Goldschmidt <goldsimon>
Group administrator
Sun 21 Oct 2007 07:34:27 PM UTC, comment #18: 

I think we should not forgot the initial problem: some errors are fatals, some others not.

We could perhaps add to netconn_xxx functions somethings like before any checkings :

if (!ERR_IS_FATAL(conn->err)) conn->err=ERR_OK;

Like this, any we "reset" any minor errors.

Frédéric Bernon <fbernon>
Group Member
Sun 21 Oct 2007 04:05:31 PM UTC, comment #17: 

To make progress on this, I've made a list of all the places where conn->err is checked:

api_lib.c:
netconn_recv -> dunno... we mustn't block on error...
netconn_send -> let do_send handle errors
netconn_write -> let do_write handle errors

api_msg.c:

do_listen -> state is already checked -> remove conn->err check
do_send -> remove conn->err check (udp_/raw_send will fail)
do_recv -> maybe check TCP state?
do_write -> check TCP state
do_join_leave_group -> ???

do_bind
 -> UDP/RAW: don't care / try again;
 -> TCP: check if pcb is in tcp_active_pcbs (not already bound or connected)


Any more ideas? Especially with do_join_leave_group and netconn_recv, I'm not really sure...

Simon Goldschmidt <goldsimon>
Group administrator
Wed 29 Aug 2007 09:30:23 AM UTC, comment #16: 

It's always "in progress" to my point of view.

About idea to check error by checking "conn->pcb" in api_lib.c, this is not safe thread, and some others function got this problem (see bug #20900: "Potential crash error problem with netconn_peer & netconn_addr").

Frédéric Bernon <fbernon>
Group Member
Wed 29 Aug 2007 09:04:58 AM UTC, comment #15: 

Is this done?  Anyone working on it?

Kieran Mansley <kieranm>
Group Member
Tue 26 Jun 2007 06:05:33 PM UTC, comment #14: 

Yes, you're right, there is a memp_free(MEMP_TCP_PCB, pcb); with each TCP_EVENT_ERR(pcb->errf, pcb->callback_arg, ERR_RST);

Frédéric Bernon <fbernon>
Group Member
Tue 26 Jun 2007 06:01:40 PM UTC, comment #13: 

About "conn->pcb.tcp = NULL;", in err_tcp, I feel that like an error: if the field is invalided, how could do_delconn do the tcp_close (and indirectly call tcp_pcb_remove)? Or tcp_pcb_remove is called by the function which call err_tcp?

But I always think that conn->err is better, since it provide a way to know why what kind of error we get. More, it's more generic when we have non-tcp netconn types...

Frédéric Bernon <fbernon>
Group Member
Tue 26 Jun 2007 05:49:20 PM UTC, comment #12: 


> since an err_tcp doesn't cause to free the pcb or the mbox


OK, not the mbox, but the pcb is deleted when err_tcp gets called, and therefore, err_tcp sets conn->pcb to NULL! That's why we have to check it! (Oh, I just realized this is not thread-safe if we do it in api_lib.c... ! -> that check must be moved to api_msg.c!)

I think the underlying functions must handle a state conflict (that can happen only for tcp anyway), which at least tcp_write() does at the moment.

So as long as we check that conn->pcb is != NULL, I think that simply calling the core function again, no matter what the last error was, is the best thing to do.


Simon Goldschmidt <goldsimon>
Group administrator
Tue 26 Jun 2007 05:38:57 PM UTC, comment #11: 

About comment #8, I should add that api_lib should never set conn->err in any case (and should just read it). But about ERR_MEM, I'm not agree: if a lwip_sendto operation doesn't get a pbuf or a buffer in the device to send a datagram, it is an error (lwip_sendto should return -1), but not a "fatal" one (the application can retry).

About comment #9 & comment #10, I think that tests like (pcb!=NULL) or (mboxes!=NULL) are not good, since an err_tcp doesn't cause to free the pcb or the mbox. But change the core to handle such cases is redundant with conn->err, and more complex than solution propose in comment #1 & comment #2.

Frédéric Bernon <fbernon>
Group Member
Tue 26 Jun 2007 04:27:30 PM UTC, comment #10: 

Sorry, forgot:

the core functions called from api_msg.c should then return an appropriate error if e.g. a tcp connection is closed. And if this is not the case, maybe we should change the core code to deal with such situations?

Simon Goldschmidt <goldsimon>
Group administrator
Tue 26 Jun 2007 04:16:12 PM UTC, comment #9: 

What if we simply ignore conn->err on entering the functions?
Shouldn't it be enough to check if all the other variable are OK (e.g. pcb!=NULL, mboxes!=NULL, etc.)?

Simon Goldschmidt <goldsimon>
Group administrator
Thu 21 Jun 2007 07:42:00 PM UTC, comment #8: 

I think for this to solve, we should never set conn->err in api_lib.c (netconn_recv currently does that) and never set it to ERR_MEM in api_msg.c.

The other solution would be to define some 'severe' errors. That would mainly be the 'connection closed' errors for TCP, I think.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 13 Jun 2007 07:01:00 PM UTC, comment #7: 

Checked in the change in pcb_new() as I said in comment #3 (returning ERR_VAL like in comment #4).

Simon Goldschmidt <goldsimon>
Group administrator
Wed 13 Jun 2007 06:04:57 PM UTC, comment #6: 

I check in change like said un comment #5...

Frédéric Bernon <fbernon>
Group Member
Tue 12 Jun 2007 01:51:35 PM UTC, comment #5: 

No objects to change return instruction in netconn_peer and netconn_addr like this ?

return ERR_OK;

(Was "return (conn->err = ERR_OK);" before)

Frédéric Bernon <fbernon>
Group Member
Mon 11 Jun 2007 08:04:08 PM UTC, comment #4: 

Right! ERR_VAL ?

Frédéric Bernon <fbernon>
Group Member
Mon 11 Jun 2007 07:55:57 PM UTC, comment #3: 

Should we set msg->conn->err to != ERR_OK in pcb_new() in the dafault case (e.g. if a protocol pcb should be created that is disabled)???

Simon Goldschmidt <goldsimon>
Group administrator
Sat 02 Jun 2007 11:11:12 AM UTC, comment #2: 


> - we also can make a "filter) for "fatal errors" like this:
> #define ERR_IS_FATAL(e) (e>ERR_BUF)
> and change the several checkings in api_lib/api_msg by :
> if (ERR_IS_FATAL(conn->err)) ...


I think something like that is definitively the best solution.
We should think about what we want to protect by not allowing an operation if conn->err != ERR_OK. The only protection I see is needed is if a connection is reset (e.g. tcp_err is called or something) and/or the pcb is definitively not in a state to accept calls.

Therefore, blocking errors should be ERR_ABRT, ERR_RST and ERR_CLSD which are indications that a connection has been closed. (Or can you re-open a closed TCP connection???)

I see no reason an application shouldn't send more and more data e.g. over a UDP socket if that socket's netif is down. It's up to the programmer to cope with that.

Simon Goldschmidt <goldsimon>
Group administrator
Sat 02 Jun 2007 10:51:28 AM UTC, comment #1: 

For netconn_peer and netconn_addr, I propose to simply replace :

return (conn->err = ERR_OK);

By

return ERR_OK;

Because there is no reason to change conn->err when we just read a state value...

For the MAC problem (out of buffers could block sending more data), if I resume some ideas in the mailing list, we can:

- not return an error in this case (finally, you also can lost packets on the wire, TCP will retransmit datas - longer than necessary - so...)

- block inside the netif's linkoutput until a buffer is available (Jonathan get a option in his port, and I add one in mine, this solution is not so bad)

- filter the ERR_MEM error like a minor error (by adding a test just after a udp_send/udp_sendto by example, and reset it to ERR_OK),

- we also can make a "filter) for "fatal errors" like this:

#define ERR_IS_FATAL(e) (e>ERR_BUF)

and change the several checkings in api_lib/api_msg by :

if (ERR_IS_FATAL(conn->err)) ...


Frédéric Bernon <fbernon>
Group Member
Fri 01 Jun 2007 05:22:37 PM UTC, original submission:  

Currently, if conn->err != ERR_OK, many functions will do nothing and simply return.

e.g. a MAC running (temporarily) out of buffers could block sending more data. In contrast, netconn_peer() resets the flag to ERR_OK...

Simon Goldschmidt <goldsimon>
Group administrator

 

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

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by kieranm (Updated the item)
  • -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 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-12-11 goldsimon StatusReady For Test Done
        Open/ClosedOpen Closed
    2007-11-27 goldsimon StatusNone Ready For Test
        Percent Complete10% 70%
        Assigned toNone goldsimon
    2007-11-27 goldsimon Planned ReleaseNone 1.3.0
    2007-11-09 kieranm Priority7 - High 5 - Normal
    2007-11-09 kieranm Planned Release1.3.0 None
    2007-08-29 kieranm Priority5 - Normal 7 - High
    2007-07-13 kieranm Priority1 - Later 5 - Normal
    2007-07-13 kieranm Planned ReleaseNone 1.3.0
    2007-06-13 goldsimon Percent Complete0% 10%

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code