patchlwIP - A Lightweight TCP/IP stack - Patches: patch #5958, netconn_listen &...

 
 

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

patch #5958: netconn_listen & netconn_accept error handling

Submitted by:  Frédéric Bernon <fbernon>
Submitted on:  Wed 23 May 2007 09:21:12 AM UTC  
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  Frédéric Bernon <fbernon> Open/Closed:  Closed
Planned Release:  None

( Jump to the original submission)

Wed 23 May 2007 06:47:21 PM UTC, comment #13: 

Ok, I check in like initially propose, with LWIP_ASSERT (I suppose than another task will be open to replace in the whole code the existing LWIP_ASSERT by the "good one").

Note that netconn_delete doesn't rise an ASSERT if conn==NULL, because on a err_tcp call, we could post/fetch a NULL netconn in acceptmbox...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 23 May 2007 02:48:12 PM UTC, comment #12: 

Oh I hadn't looked at Kieran's comments there. I'll reply about that in that task. LWIP_DEBUG_ASSERT reflects what's in CVS at least.

Jonathan Larmour <jifl>
Project Member
Wed 23 May 2007 02:36:03 PM UTC, comment #11: 

And I thought we wanted to get rid of it again and have ASSERT for error catching and program flow to catch error which can happen in a well-tested realease mode application. That would imply that ASSERTs only fire when debugging -> through programmer errors. And then, we would not need the DEBUG_ASSERT. That was what I thought we discussed in tark #6792... Only we didn't finish that discussion with a final statement...

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 23 May 2007 02:25:57 PM UTC, comment #10: 

I don't understand. I thought LWIP_DEBUG_ASSERT was precisely for this sort of programmer error?

Jonathan Larmour <jifl>
Project Member
Wed 23 May 2007 02:07:58 PM UTC, comment #9: 

I think we have somehow agreed in that we want to be able to catch all errors that don't come from a programmer's fault using normal code flow (return values) and remove the LWIP_DEBUG_ASSERT...
So you might better use the normal LWIP_ASSERT...

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 23 May 2007 02:03:10 PM UTC, comment #8: 

Since everyone seems ok, I will replace them by LWIP_DEBUG_ASSERT.

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 23 May 2007 01:58:41 PM UTC, comment #7: 

Re comment #5, I think yes. In fact these would want Simon's new LWIP_DEBUG_ASSERT.

Jonathan Larmour <jifl>
Project Member
Wed 23 May 2007 01:53:23 PM UTC, comment #6: 

Yep, assertions sounds good to me.

Kieran Mansley <kieranm>
Project Member
Wed 23 May 2007 01:43:45 PM UTC, comment #5: 

I agree with you, but if you want to be coherent, we also should replace in api_lib function any code like ...

if (conn == NULL) {
  return ERR_VAL;
}

... by an assert, right?

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 23 May 2007 01:41:00 PM UTC, comment #4: 

That's also true... an ASSERT should be enough.

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 23 May 2007 01:31:10 PM UTC, comment #3: 

Since this indicates a coding error, rather than something that can sometimes happen in real use, should this be an assert?

Jonathan Larmour <jifl>
Project Member
Wed 23 May 2007 09:50:26 AM UTC, comment #2: 

in api_lib.c, in netconn_accept, it will be :

  if (conn->acceptmbox == SYS_MBOX_NULL) {
    conn->err = ERR_VAL;
    return NULL;
  }

and in sockets.c, in lwip_accept, it will be :

  newconn = netconn_accept(sock->conn);
  if (!newconn) {
    LWIP_DEBUGF(SOCKETS_DEBUG, ("lwip_accept(%d) failed, err=%d\n", s, sock->conn->err));
    sock_set_errno(sock, err_to_errno(sock->conn->err));
    return -1;
  }

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 23 May 2007 09:25:08 AM UTC, comment #1: 

agree

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 23 May 2007 09:21:12 AM UTC, original submission:  

if your call netconn_accept without any previous netconn_listen call, the acceptmbox is not valide, and you can have an error (consequence is "port dependant"). I propose to add a simple guard inside netconn_accept:

if (conn->acceptmbox == SYS_MBOX_NULL) {
  return ERR_VAL;
}

For "listen", the code to allocate the acceptmbox is done inside netconn_listen AND inside do_listen. I propose to remove the code inside netconn_listen, because there is a better error handling in do_listen (netconn type check, check tcp_listen return values...)

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.

 

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 kieranm (Posted a comment)
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by fbernon (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-05-23 fbernon StatusNone => Done
        Open/ClosedOpen => Closed

    Back to the top


    Powered by Savane 3.5