buglwIP - A Lightweight TCP/IP stack - Bugs: bug #19345, tcpip_apimsg+api_msg_post problem

 
 

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

bug #19345: tcpip_apimsg+api_msg_post problem

Submitted by:  Frédéric Bernon <fbernon>
Submitted on:  Mon 19 Mar 2007 04:53:45 PM UTC  
 
Category: NoneSeverity: 4 - Important
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: Frédéric Bernon <fbernon>
Open/Closed: ClosedPlanned Release: None
lwIP version: None

(Jump to the original submission Jump to the original submission)

Wed 21 Mar 2007 05:09:34 PM UTC, comment #8:

Commited with:

void
api_msg_input(struct api_msg *msg)
{ struct api_msg_msg msg_copy;
msg_copy=msg->msg;
decode[msg->type](&msg_copy);
}

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 21 Mar 2007 05:09:06 PM UTC, comment #7:

>If you use the local variable to send tcpip_msg, it would still leave
>you with potential problems. because you didn't create an copy for this
>variable, the local variable will be unavliable when the program run to
>other area.


It "could be" true, but don't forget the tcpip_apimsg's call is block (inside the "struct tcpip_msg msg;" scope) by "sys_mbox_fetch(apimsg->msg.conn->mbox, NULL)" until tcpip_thread post it a "acknowledge" (and in all api_msg "do_xxx"; the "sys_mbox_post(msg->conn->mbox, NULL);" is "always" at the end). So, the variable can't be used outside the scope of tcpip_apisg. The "generic" process is that (simplified) :

App Thread
tcpip_apimsg()
{ struct tcpip_msg msg
post to tcpip_thread
sys_mbox_fetch(start wait...

tcpip_thread()
{ sys_mbox_fetch()
api_msg_input()
do_xxxx()
{ // message process
sys_mbox_post(msg->conn->mbox, NULL);
}
}
End wait...)
}

I say "always", but there is some exceptions. This is my check list :

do_newconn => sys_mbox_post(msg->conn->mbox, NULL) at the end, but if (msg->conn->pcb.tcp != NULL), it is called immediatly
do_delconn => sys_mbox_post(msg->conn->mbox, NULL) at the end, but with a (msg->conn->mbox != SYS_MBOX_NULL) check
do_bind => sys_mbox_post(msg->conn->mbox, NULL) at the end
do_connect => multiples sys_mbox_post(msg->conn->mbox, NULL) with return just after
do_disconnect => sys_mbox_post(msg->conn->mbox, NULL) at the end
do_listen => sys_mbox_post(msg->conn->mbox, NULL) at the end
do_accept => !!!! no "direct" process? And YES, this can be a problem... But in fact, do_accept do nothing, can its call could be avoid, because never use (will have to be suppress)...
do_send => sys_mbox_post(msg->conn->mbox, NULL) at the end
do_recv => sys_mbox_post(msg->conn->mbox, NULL) at the end
do_write => sys_mbox_post(msg->conn->mbox, NULL) at the end
do_close => sys_mbox_post(msg->conn->mbox, NULL) at the end
do_join_leave_group => sys_mbox_post(msg->conn->mbox, NULL) at the end

Some extras calls:
do_connected: => when a tcp connection is really connected, sys_mbox_post(msg->conn->mbox, NULL) est call.
err_tcp => called when tcp error, and all conn's mailboxes receive a NULL message (recvmbox, mbox, acceptmbox).

Last, I you really want to be sure, you can write something like :

void
api_msg_input(struct api_msg *msg)
{ struct api_msg_msg msg_copy;
msg_copy=msg->msg;
decode[msg->type](&msg_copy);
}

This will stay faster than using memp...

About this part, agree to commit?

>Do you meet the condition that MEMP_TCP_SEG can not be free in some
>case.

I don't understand this question (in this context) ? But no, I don't meet this condition. Of course, I will tell you if I see something like that...

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Wed 21 Mar 2007 11:55:16 AM UTC, comment #6:

Seems OK to me.

Kieran Mansley <kieranm>
Project Administrator
Tue 20 Mar 2007 07:44:50 PM UTC, comment #5:

I join a patch to solve the problem, and to improve reliability (and code is more simple).

"api_lib.c, tcpip.c: integrate sys_mbox_fetch(conn->mbox, NULL) calls from api_lib.c to tcpip.c's tcpip_apimsg(). Now, use a local variable and not a dynamic one from memp to send tcpip_msg to tcpip_thread in a synchrone call. Free tcpip_msg from tcpip_apimsg is not done in tcpip_thread. This give a faster and more reliable communication between api_lib and tcpip."

Patch file is attached.

If no objection, I will commit it tomorrow...

(file #12223)

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Tue 20 Mar 2007 09:55:35 AM UTC, comment #4:

Other solution (if someone read it?):

remove MEMP_TCPIP_MSG's alloc/free feature. Because only "api" used it we could change like this :

err_t
tcpip_apimsg(struct api_msg *apimsg)
{
struct tcpip_msg msg;
msg->type = TCPIP_MSG_API;
msg->msg.apimsg = apimsg;
sys_mbox_post(mbox, msg);
sys_mbox_fetch(apimsg->msg.conn->mbox, NULL);
return ERR_OK;
}

and change at the en of tcpip_thread this line:

memp_free(MEMP_TCPIP_MSG, msg);

by this one :

if (msg->type!=TCPIP_MSG_API)
memp_free(MEMP_TCPIP_MSG, msg);

It will be faster, footprint will not be increased and more reliable (no "not enought memory" problem...). Even, I suggest to directy call tcpip_apimsg from api_lib.c, because it would be faster and design would be simple (or we can use a define?)...

Hoping comments ?

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Tue 20 Mar 2007 08:27:21 AM UTC, comment #3:

api_msg_post() is only used in api_lib.c. Each call is the same sequence:

//Prepare message
api_msg_post(&msg);
sys_mbox_fetch(conn->mbox, NULL);

I propose to change api_msg_post like this :

err_t
api_msg_post( struct netconn conn, struct api_msg msg)
{ msg->msg.conn = conn;
if (tcpip_apimsg(msg)==ERR_OK)
{ sys_mbox_fetch(conn->mbox, NULL);
}
return ERR_MEM;
}

And to change each call in api_lib.c like this:

err_t
netconn_disconnect(struct netconn *conn)
{
struct api_msg msg;

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

msg.type = API_MSG_DISCONNECT;
api_msg_post( conn, &msg);

return conn->err;

}

Your comments?

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Mon 19 Mar 2007 08:37:04 PM UTC, comment #2:

First step is done. Before change api_lib.c, is there any comments?

Frédéric Bernon <fbernon>
Project MemberIn charge of this item.
Mon 19 Mar 2007 05:09:31 PM UTC, comment #1:

Sounds like a good idea to me.

Kieran Mansley <kieranm>
Project Administrator
Mon 19 Mar 2007 04:53:45 PM UTC, original submission:

tcpip_apimsg & api_msg_post functions don't have any return type. So, if we got any memp_malloc error, api_msg_post's caller doesn't know that the message is not sent. And in api_lib.c, we always wait an answer with a sys_mbox_fetch(conn->mbox, NULL); But tcpip_thread will never answers, because he never got the message.

This is not a new problem. I can find in forum some threads :

- "bug&patch: when an api_msg is lost tcp get stuck" (May 2005)
- "API code issues" (May 2006)

The first thing to change in api is to change return types from "void" to "err_t" for tcpip_apimsg & api_msg_post (like this):

err_t
tcpip_apimsg(struct api_msg *apimsg)
{
struct tcpip_msg *msg;
msg = memp_malloc(MEMP_TCPIP_MSG);
if (msg == NULL) {
return ERR_MEM;
}
msg->type = TCPIP_MSG_API;
msg->msg.apimsg = apimsg;
sys_mbox_post(mbox, msg);
return ERR_OK;
}

err_t
api_msg_post(struct api_msg *msg)
{
return tcpip_apimsg(msg);
}

The second thing is to change in api_lib.c each call like this :

...
if (api_msg_post(&msg)==ERR_OK)
{ sys_mbox_fetch(conn->mbox, NULL);
...
}
else
{ ... //error handler
}
...

Good for you?

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

 

Attached Files
file #12223:  tcpip_apimsg.patch added by fbernon (6KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by kieranm (Posted a comment)
  • -unavailable- added by fbernon (Submitted the item)
  •  

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

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 3 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 21 Mar 2007 05:09:34 PM UTCfbernonStatusIn Progress=>Fixed
      Open/ClosedOpen=>Closed
    Tue 20 Mar 2007 07:44:50 PM UTCfbernonAttached File-=>Added tcpip_apimsg.patch, #12223

    Back to the top


    Powered by Savane 3.1-cleanup1