tasklwIP - A Lightweight TCP/IP stack - Tasks: task #10369, Various changes

 
 

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

task #10369: Various changes

Submitted by:  Iordan Neshev <iordan_neshev>
Submitted on:  Wed 05 May 2010 08:28:40 PM UTC  
 
Category:  None Should Start On:  Wed 05 May 2010 12:00:00 AM UTC
Should be Finished on:  Wed 05 May 2010 12:00:00 AM UTC Priority:  5 - Normal
Status:  Done Privacy:  Public
Percent Complete:  100% Assigned to:  Simon Goldschmidt <goldsimon>
Open/Closed:  Closed Planned Release:  None
Effort:  0.00

Wed 12 May 2010 07:48:37 AM UTC, comment #4:

>> change pbuf->ref from u16_t to s16_t to check for underflows?
> We'd better not. After all, the nature of ref is unsigned.

I think so, too.

>> Add a SIO_FD_X printf-formatter define
> Yet another formatter? Casting to u32_t would do the job


On 64bit architectures, you might get a warning when casting a pointer to 32bit. Instead, I'll leave the cast to size_t and use SZT_F as the printf-formatter. On 32bit platforms, this should be the same.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 12 May 2010 07:25:43 AM UTC, comment #3:

>change pbuf->ref from u16_t to s16_t to check for underflows?

We'd better not. After all, the nature of ref is unsigned.

>Add a SIO_FD_X printf-formatter define

Yet another formatter? Casting to u32_t would do the job
-----

> 2. ppp_oe.c

oh, you're right. I didn't look at it from this point of view. Sorry if we have already talked about this, I don't remember.

Iordan Neshev <iordan_neshev>
Tue 11 May 2010 06:14:03 PM UTC, comment #2:

That leaves us with:
- change pbuf->ref from u16_t to s16_t to check for underflows? (can't happen as long as anyone messes around with ->ref without using the pbuf_* functions)

- Add a SIO_FD_X printf-formatter define?

For the rest, I'd wait for changes in pppd.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 11 May 2010 06:12:15 PM UTC, comment #1:

> 1. In file pbuf.c, line 566 there is ...


I don't think that casting to s32_t changes anything since it would result in an s32_t between 0 and 0x0000ffff, which would never be negative. Casting to s16_t might work, but would limit the current ref-count ot 32K, which shouldn't be a problem, but still, if it was meant like that, I'd rather change the structure definition to s16_t.

> 2. ppp_oe.c
> sys_untimeout should be UNTIMEOUT (lines 286, 618, 631 and 1211)


Hm, ppp_oe.c is a file written only for lwIP and my opinion is that it therefore doesn't have to use the compatibility-defines (UN)TIMEOUT, which were introduced for the pppd code (where TIMEOUT first calls sys_untimeout and then sys_timeout). (After all, if UNTIMEOUT was used, than we would have to use TIMEOUT, too...)

> 3. ppp.c : nPut(PPPControl pc, struct pbuf nb)


Hmm, that one is tricky. The current cast isn't correct since size_t is unsigned (and may be larger than expected for %d) while %d expects (signed-)int. However, we don't have any way of knowing which printf-formatter is correct for sio_fd_t, so for fixing this correctly, we would have to introduce a SIO_FD_X define.

> 4. ppp/fsm.c: function fsm_sconfreq(fsm *f, int retransmit)


I'd agree with you there, but that's not the only place where the present progressive is used instead of the simple past, and I wouldn't just change it in one place...

> 5. ...


Wouldn't we write beyond the buffer (and thus overwrite another memory area) with p[len] = 0 if len is the size of the buffer that p points to???

Aside from that, I wouldn't want to change that unless it's in changed in pppd: I'm not planning lwIP's ppp to become a real fork of the original pppd with lots of changes (after all, if I had the time, I'd completely upgrade to 2.4.5 and use ifdef/macros to disable the stuff we don't need...).

> 6. ...


I've added the call to lcp_close. In lwIP, the state is changed by calling pppIOCtl, so this is already covered.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 05 May 2010 08:28:40 PM UTC, original submission:

I have several proposals:

1. In file pbuf.c, line 566 there is
LWIP_ASSERT("pbuf_free: p->ref > 0", p->ref > 0);

Since p->ref is decremented on the next line, if underflow
occurs, the assert would not fire. Casting to s32_t (or maybe s16_t to be more lightweight?) should solve this:
LWIP_ASSERT("pbuf_free: p->ref > 0", (s32_t)p->ref > 0); /* cast to s32 to detect u16_t underflow */

2. ppp_oe.c
sys_untimeout should be UNTIMEOUT (lines 286, 618, 631 and 1211)

3. ppp.c : nPut(PPPControl pc, struct pbuf nb)

there is: ("PPP nPut: incomplete sio_write(fd:%d, len:%d, c: 0x%"X8_F") c = %d\n", (size_t)pc->fd, b->len, c, c));

pc->fd should be casted to sio_fd_t, not size_t.

4. ppp/fsm.c: function fsm_sconfreq(fsm *f, int retransmit)

on the bottom there is:
FSMDEBUG(LOG_INFO, ("%s: sending Configure-Request, id %d\n",

I believe, we should rename "sending" to "sent", because it is quite misleading when you have to read the PPP log, although pppd says "sending". I'll send this to the pppd mailing list too.

5. ppp/fsm.c: fsm_rtermreq(fsm f, int id, u_char p, int len)

There is
case LS_OPENED:
if (len > 0) {
FSMDEBUG(LOG_INFO, ("%s terminated by peer (%p)\n", PROTO_NAME(f), p));
} else {
FSMDEBUG(LOG_INFO, ("%s terminated by peer\n", PROTO_NAME(f)));
}

I propose to change it like this:
if ((len > 0) && (p != NULL)) {
p[len] = '\0'; /* Terminate remote message */
FSMDEBUG(LOG_INFO, ("%s terminated by peer (%s)\n", PROTO_NAME(f), p));
} else {
FSMDEBUG(LOG_INFO, ("%s terminated by peer\n", PROTO_NAME(f)));
}

I've seen it with my eyes that the remote message should be terminated locally, otherwise random garbage appears after it. Also, p may be renamed to msg, because we use p for pbuf, not for char*.

6. ppp/auth.c: auth_withpeer_fail(int unit, u16_t protocol)
In the latest pppd this function has these 2 rows added at the bottom:
status = EXIT_PEER_AUTH_FAILED;
lcp_close(unit, "Authentication failed");

We do not use currently status in PPP code, but I think
lcp_close(unit, "Authentication failed");
should be added at the end.

Sorry for not preparing diff files, but my lwip files are pretty different from the CVS ones.

Iordan Neshev <iordan_neshev>

 

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 goldsimon (Posted a comment)
  • -email is unavailable- added by iordan_neshev (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 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-05-12 goldsimon StatusNone => Done
        Percent Complete0% => 100%
        Assigned toNone => goldsimon
        Open/ClosedOpen => Closed

    Back to the top


    Powered by Savane 3.3