tasklwIP - A Lightweight TCP/IP stack - Tasks: task #7142, Sanity check user-configurable...

 
 

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

task #7142: Sanity check user-configurable values

Submitter:  Jonathan Larmour <jifl>
Submitted:  Thu 02 Aug 2007 07:52:12 PM UTC
   
 
Category:  None Should Start On:  Wed 01 Aug 2007 11:00:00 PM UTC
Should be Finished on:  Wed 01 Aug 2007 11:00:00 PM UTC Priority:  7 - High
Status:  Done Privacy:  Public
Assigned to:  jifl Percent Complete:  100%
Open/Closed:  Closed Planned Release:  1.3.0
Effort:  0.00

Jump to the original submission

Fri 21 Sep 2007 02:58:47 PM UTC, comment #35: 

Wow, that's a strange #if...

I can live with a strange error report if people set those defines to anything other than 0 or 1 :-)

Simon Goldschmidt <goldsimon>
Group administrator
Fri 21 Sep 2007 02:45:31 PM UTC, comment #34: 

I believe the values for these particular options should never be anything other than 0 or 1.

Jonathan Larmour <jifl>
Group Member
Fri 21 Sep 2007 02:42:41 PM UTC, comment #33: 

Ok for me. Just a remark: if "options" are not 0 or 1 (I don't know, any strange use of expression where the value is >1), it can give a strange behavior...

Frédéric Bernon <fbernon>
Group Member
Fri 21 Sep 2007 02:37:51 PM UTC, comment #32: 

I'm adding the attached patch too, to check there are sufficient timeouts.

(file #14004)

Jonathan Larmour <jifl>
Group Member
Wed 12 Sep 2007 01:17:18 PM UTC, comment #31: 

Looks right to me.

Jonathan Larmour <jifl>
Group Member
Wed 12 Sep 2007 09:41:16 AM UTC, comment #30: 

I would like to know if - with current code - it's wrong to suppose that slipif and ppp need NO_SYS=0? I ask this because I see they use sys_thread_new.

For slipif, I think it's not a problem (the processing in slipif_loop can be done in the "single-thread" model). We could perhaps add #if !NO_SYS/#endif around slipif_loop() just to signal this function is used when you got an OS.

But, for PPP, it seems that pppMain do lot of job in "blocking" while. More, there is some calls to tcpip_callback, timeout, etc... So, I suppose we should add some others sanity check for that:

#if (PPP_SUPPORT && (NO_SYS==1))
  #error "If you want to use PPP, you have to define NO_SYS=0 in your lwipopts.h"
#endif

Frédéric Bernon <fbernon>
Group Member
Fri 07 Sep 2007 11:30:48 PM UTC, comment #29: 

Ok, since last changes are checking (and I hope, most of contrib modules updated), I close this task...

Frédéric Bernon <fbernon>
Group Member
Fri 07 Sep 2007 08:52:57 PM UTC, comment #28: 

I'm happy with what you suggest in comment #21 if they are now obsolete.

I guess I can't think of any more checks at the moment, and more importantly, we now have a place and template to add the checks. So I'm ok to close this task and we'll just add more as they occur to us.

Jifl

Jonathan Larmour <jifl>
Group Member
Fri 07 Sep 2007 06:01:15 PM UTC, comment #27: 

Ok, it's check in. Any thoughts about comment #21, or can I close this task ?

Frédéric Bernon <fbernon>
Group Member
Fri 07 Sep 2007 01:48:03 PM UTC, comment #26: 

What you say is true. You can put them into the 'global' check section.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 05 Sep 2007 12:57:54 PM UTC, comment #25: 

For ARP_TABLE_SIZE, it's not really references outside etharp.c (in fact, this is the index used to access to the table which are limited to s8_t range).

In etharp_init, there is also the same kind of assert (which could/should be removed).

But there is also a dependency with IP_PCB_ADDRHINT which is defined in ip.h. So, if both etharp.h and ip.h have the same limitation, centralized the checking is perhaps better ? And if one day, etharp.h/.c and ip.h are changed to use s16_t, at the first test with ARP_TABLE_SIZE>0x7f, the developer who do the change will got the #error, so, it seems difficult to miss that.

Same for tcp.h: we already check this value in lwip_sanity_check (but like a "Warning") :

  if (TCP_WND > (PBUF_POOL_SIZE*PBUF_POOL_BUFSIZE))
    LWIP_PLATFORM_DIAG(("lwip_sanity_check: WARNING: TCP_WND is larger than space provided by PBUF_POOL_SIZE*PBUF_POOL_BUFSIZE\n"));
  if (TCP_WND < TCP_MSS)
    LWIP_PLATFORM_DIAG(("lwip_sanity_check: WARNING: TCP_WND is smaller than MSS\n"));

Since there is already check on TCP_WND in this place, it could be better to centralized?

So, Simon, for you, is it a objection, or just a remark?

Frédéric Bernon <fbernon>
Group Member
Wed 05 Sep 2007 12:29:30 PM UTC, comment #24: 

(Simon wrote :)

> in tcp.h:
>
> #if TCP_WND > 0xffff
> #error TCP_WND must fit in an u16_t
> #endif
>
> in etharp.h:
>
> #if ARP_TABLE_SIZE > 0x7f
> #error ARP_TABLE_SIZE must fit in an s8_t #endif


Since those are directly related to the type of the array, I'd suggest to leave them near the declaration. If someday the declaration type is changed, it's more likely the test is also changed if it's directly above the declaration...

Frédéric Bernon <fbernon>
Group Member
Wed 05 Sep 2007 11:04:00 AM UTC, comment #23: 

Seems fine to me. Go for it!

Jonathan Larmour <jifl>
Group Member
Wed 05 Sep 2007 10:35:22 AM UTC, comment #22: 

If we want to centralize other sanity checkings, there also :

in tcp.h:

#if TCP_WND > 0xffff
#error TCP_WND must fit in an u16_t
#endif

in etharp.h:

#if ARP_TABLE_SIZE > 0x7f
#error ARP_TABLE_SIZE must fit in an s8_t
#endif

in inet.c:

#ifndef BYTE_ORDER
#error BYTE_ORDER is not defined
#endif

in autoip.c:

#if !LWIP_ARP
#error LWIP_ARP is need for LWIP_AUTOIP. Set it from your lwipopts.h.
#endif /* !LWIP_ARP */

in msg_in.c :

#if (SNMP_CONCURRENT_REQUESTS == 0)
#error "need at least one snmp_msg_pstat"
#endif

in msg_out.c :
#if (SNMP_TRAP_DESTINATIONS == 0)
#error "need at least one trap destination"
#endif

Thoughts?

Frédéric Bernon <fbernon>
Group Member
Fri 31 Aug 2007 08:44:29 AM UTC, comment #21: 

There is also : MEMP_NUM_API_MSG, SYS_STATS, RAW_STATS, and some others removed from opt.h in https://savannah.nongnu.org/task/?7143.

Perhaps we could add them to "deprecated options" ?

Frédéric Bernon <fbernon>
Group Member
Fri 31 Aug 2007 08:29:52 AM UTC, comment #20: 

OK.

Kieran Mansley <kieranm>
Group Member
Fri 31 Aug 2007 08:20:48 AM UTC, comment #19: 

There are :

/* Compile-time checks for deprecated options.
 */
#ifdef MEMP_NUM_TCPIP_MSG
  #error MEMP_NUM_TCPIP_MSG option is deprecated. Remove it from your lwipopts.h.
#endif
#ifdef ETHARP_QUEUE_FIRST
  #error ETHARP_QUEUE_FIRST option is deprecated. Remove it from your lwipopts.h.
#endif
#ifdef ETHARP_ALWAYS_INSERT
  #error ETHARP_ALWAYS_INSERT option is deprecated. Remove it from your lwipopts.h.
#endif
#ifdef SO_REUSE
/* I removed the lot since this was an ugly hack. It broke the raw-API.
   It also came with many ugly goto's, Christiaan Simons. */
  #error "SO_REUSE currently unavailable, this was a hack"
#endif

SO_REUSE could be reactivate in the future, since it's a feature useful (we have open "task #6995: Implement SO_REUSEADDR" for do that in the future).

Frédéric Bernon <fbernon>
Group Member
Fri 31 Aug 2007 08:16:14 AM UTC, comment #18: 

Can you briefly outline which are the deprecated options?

Kieran Mansley <kieranm>
Group Member
Thu 30 Aug 2007 06:40:25 PM UTC, comment #17: 

ok for me

Frédéric Bernon <fbernon>
Group Member
Thu 30 Aug 2007 06:25:09 PM UTC, comment #16: 

I see no reason not to. It seems like a good idea to me, especially with so much upheaval in 1.3.

Jonathan Larmour <jifl>
Group Member
Thu 30 Aug 2007 06:12:12 PM UTC, comment #15: 


> I have made some more changes to this file to add some more checks and to move all the compile-time checks out of the LWIP_DEBUG ifdef block.


Since we're doing compile-time checks here, can we move all deprecated options into here? It would be nice to clean up opt.h to ONLY include what is current -- then you can copy opt.h to lwipopts.h and only get the "good stuff".

Jared Grubb <jgrubb>
Group Member
Thu 30 Aug 2007 03:42:10 PM UTC, comment #14: 

I have made some more changes to this file to add some more checks and to move all the compile-time checks out of the LWIP_DEBUG ifdef block.

I realised some of the checks were to compare with the number of pbufs usable for outgoing packets, but of course we don't know how many PBUF_RAMs will be created. We could put a maxmimum on that value by dividing down MEM_SIZE, but there doesn't seem much point. So I have removed some of the TODOs.
 
There's still a check for the number of pool pbufs and TCP_WND though as I think that's still valid for incoming packets.

Jonathan Larmour <jifl>
Group Member
Thu 30 Aug 2007 03:11:01 PM UTC, comment #13: 

No, since if LWIP_DEBUG is undefined, you got :

#define lwip_sanity_check()

in line 123. ;)

Frédéric Bernon <fbernon>
Group Member
Thu 30 Aug 2007 02:52:23 PM UTC, comment #12: 

Looks good so far.

Needs #ifdef LWIP_DEBUG in lwip_init around the check call tho :)

Jared Grubb <jgrubb>
Group Member
Thu 30 Aug 2007 10:20:55 AM UTC, comment #11: 

Ok, I have check in a first version for lwip_sanity_check, "as is".

Jonathan, I can join you on IRC, so, I let you do your changes directly (I think it will be better), because I should have some meetings today and tomorrow. So, change it like you think it's better, there is no problem for me.




Frédéric Bernon <fbernon>
Group Member
Thu 30 Aug 2007 10:03:29 AM UTC, comment #10: 

I don't see any reason to remove the  "if (MEMP_NUM_TCP_SEG < TCP_SND_QUEUELEN)" check. If someone has configured it that way, it's definitely wrong. It doesn't matter that TCP_SND_QUEUELEN is for a single PCB in that sense. For multiple PCBs the problem would be worse not better!

Thanks for changing the two I mentioned, although the tests should be ">=", not just ">".

I'm happy for it to be checked in for other people to change (after these little fixes). I can add more of the things from the initial post.

For the LWIP_DEBUG thing - LWIP_DEBUG is not on by default, so I don't think it's true that new users will get these.

But if people decide they do want it to be a separate option, then only the run-time checks need be conditionally compiled. The checks that can be done by the preprocessor can stay in since in fact they will not be present if unused.

Jonathan Larmour <jifl>
Group Member
Thu 30 Aug 2007 08:06:39 AM UTC, comment #9: 

It seems I misunderstand you for this one. I attach a new lwip_sanity_check function in a file.

Note I have "remove" the if (MEMP_NUM_TCP_SEG < TCP_SND_QUEUELEN), since if seems difficult to compare a global resource with a "per pcb" resource (like for comment #3).

I also add some other checkings (for ARP, DHCP and AUTOIP use).

There is a @todo comment part: this is some points to see.

Feel free to add/modify this function. Perhaps it could be good to directly check in to let any active developer change it? No objections?

Last, actually this function is executed if LWIP_DEBUG is set. Perhaps it would be good to use a LWIP_SANITY_CHECK_USER_OPTION set per default to 1 in opt.h. Like this, without any action to do, new users got these checkings.




(file #13837)

Frédéric Bernon <fbernon>
Group Member
Wed 29 Aug 2007 11:26:56 PM UTC, comment #8: 

Thanks for this. Some comments on the patch:

  • if (MEMP_NUM_NETCONN < (all the PCBs)


This is the opposite sense from what I had been thinking of. It's possible to configure fewer netconns than PCBs (e.g. if using the raw API, or raw API subsystems like SNMP too), but it would be pointless to configure more. Not incorrect, but pointless possibly revealing a lack of understanding of the config dependencies.

  •  if (MEMP_NUM_NETBUF < (PBUF_POOL_SIZE+MEMP_NUM_PBUF))


Ditto here. It's possible to have fewer netbufs, but pointless to have more.

The rest looks good, and this makes it easy to add more, thanks!

Jonathan Larmour <jifl>
Group Member
Wed 29 Aug 2007 10:06:45 PM UTC, comment #7: 

I attach here a first patch file for these sanity check. Comments?

(file #13833)

Frédéric Bernon <fbernon>
Group Member
Wed 29 Aug 2007 09:22:49 AM UTC, comment #6: 

No, lwip_init is checked in, but not include - yet - this feature, but it can/should be the right place to add these sanity checkings, since some of them can only be done at runtime (the "warnings"). To give background, I copy/paste here the part of IRC chat where we talk about that with Jonathan:


[17:26] <fbernon> I would like to add at the end of opt.h some of the checks about "task #7142 : Sanity check user-configurable values"
[17:26] <fbernon> Since they can avoid some questions
[17:27] <fbernon> is it ok for you ?
[17:36] <jifl> Sure, fire away
[17:37] <jifl> I'd probably put some demarcation and say "no user edittable stuff below this line"
[17:37] <jifl> So users don't think it's stuff they need to change
[17:39] <fbernon> if they copy/paste opt.h to create their lwipopts.h ?
[17:44] <fbernon> or we could include a "optcheck.h" at the end of opt.h ?
[17:47] <jifl> if i were a user, i would copy/paste opt.h to make lwipopts.h.
[17:47] <jifl> (personally)
[17:47] <jifl> optcheck.h might be an idea. I'm fine with that.
[17:48] <fbernon> ok
[17:48] <fbernon> is there any standard preprocessor instruction, like #error, but to only display a message ?
[17:49] <fbernon> which is C ANSI, of course...
[17:49] <jifl> no
[17:50] <jifl> I think #warning might be in C99
[17:50] <jifl> but not C90
[17:50] <jifl> so that would not be portable
[17:50] <jifl> for any warnings, I was going to check those at runtime
[17:50] <jifl> Put them in a #if LWIP_DEBUG block in sys_init() or something like that
[17:51] <fbernon> If we had a lwip_init, it would be the good place
...
[17:54] <fbernon> is there any standard define like "__cplusplus" to know if the compiler is C99 compliant ?
[17:56] <jifl> Yes
[17:56] <jifl> I'll dig it up
[17:57] <jifl> STDC_VERSION
[17:58] <jifl> This will be 199901L for C99
[17:59] <fbernon> So, we can do "warning" check in a "#if STDC_VERSION / #endif" block ?
[17:59] <jifl> C90 doesn't define it
[18:00] <fbernon> ok, old compiler won't do these checks, but it's just a "helper" after all...
[18:01] <jifl> I'm not sure that would work. I think the preprocessor still expects to be able to parse lines beginning with '#'
[18:01] <jifl> Or at least, not work portably
[18:01] <jifl> I still think it's better to check for things to warn about at runtime in one of the init functions
[18:03] <fbernon> problem: if you don't activate LWIP_DEBUG, you don't have these check
[18:03] <jifl> That's true.
[18:05] <jifl> I was going to suggest that LWIP_DEBUG be on by default. But I see something more odd - LWIP_DEBUG isn't in opt.h at all.
[18:05] <fbernon> yes
[18:06] <fbernon> most of time, it is checked by #ifdef and not #if
[18:06] <fbernon> same for LWIP_NOASSERT
[18:06] <jifl> It should probably be in opt.h, so people know it exists to change in lwipopt.h
[18:06] <jifl> lwipopts.h
[18:06] <fbernon> I don't find that very nice
[18:07] <jifl> Even if commented out
[18:07] <fbernon> replace in debug.h the #ifdef LWIP_DEBUG... by a #if LWIP_DEBUG will be better
[18:08] <fbernon> but it's another port-breakage
[18:08] <jifl> I agree. Although 1.3.0 is about port breakage :-)
[18:09] <fbernon> yes
[18:09] <fbernon> before change that, I will post the change on task #7142

[18:21] <fbernon> we are talking about put LWIp_DEBUG and LWIP_NOASSERT in opt.h, with a change in debug.h
[18:21] <fbernon> but it cause another port-breakage
[18:22] <fbernon> this idea is to get LWIP_DEBUG per default, and to do all sanity check at runtime in sys_init (or any other)
[18:23] <jaredgrubb> ok
[18:23] <fbernon> I talk about task #7142
[18:23] <jaredgrubb> i havent used the debug in lwip at all... so i may not be much help there
[18:23] <jifl> it doesn't make much difference if everything works :-)
[18:24] <fbernon> and of course, all works fine at the first try with lwIP ;)
[18:24] <jifl> All the individual modules to be debugged have their debug output set at LWIP_DBG_OFF
[18:24] <jifl> (by default)
[18:25] <fbernon> yes, but even like this, you add the code of the LWIP_DEBUG macro
[18:25] <jifl> Sort of - for a disabled module, the macro would become something like:
[18:26] <jifl> do { if ((((0) & 0x80U) && ......
[18:26] <fbernon> the code won't do anything, since you got a if (0), but, except some good compiler/linker, the code is here
[18:26] <jifl> any optimising compiler will get rid of that
[18:27] <fbernon> I'm agree

Frédéric Bernon <fbernon>
Group Member
Wed 29 Aug 2007 09:02:13 AM UTC, comment #5: 

Given that the lwip_init function has gone in now, is this also fixed?

I'd like to get these suggestions in for 1.3.0

Kieran Mansley <kieranm>
Group Member
Wed 15 Aug 2007 10:04:58 PM UTC, comment #4: 

I have open a "task #7213 : Add a lwip_init function" where we have talk about the place where do these sanity check, since some of them (the "warnings") can only be done at runtime...

Frédéric Bernon <fbernon>
Group Member
Fri 03 Aug 2007 03:17:52 PM UTC, comment #3: 


>>TCP_WND versus PBUF_POOL_SIZE*PBUF_POOL_BUFSIZE
>
>Sure of that? TCP_WND isn't "per PCB", vs a "global" PBUF_POOL" is
>shared by all pcb ?


Yes, but you can still check that there is enough pbuf pool capacity to support TCP_WND for a single connection. We can't suppose to guess about more than that, but just one should be reasonable to verify. Are am I forgetting something?

Jonathan Larmour <jifl>
Group Member
Fri 03 Aug 2007 08:03:32 AM UTC, comment #2: 

- Error if !UDP && DHCP
- Error if !UDP && IGMP

- Error if UDP && MEMP_NUM_UDP_PCB=0
- Error if TCP && MEMP_NUM_TCP_PCB=0

- Warning if MEMP_NUM_(NETBUF|NETCONN|TCPIP_MSG)!=0 and NO_SYS=1
- Warning if MEMP_NUM_(NETBUF|NETCONN|TCPIP_MSG)!=0 and NO_SYS=1

- Warning/Error if MEMP_SYS_TIMEOUT!=0 and NO_SYS=0 and we build tcpip.c ???

>Perhaps a warning if DHCP && AUTOIP && !DHCP_AUTOIP_COOP


It can be a normal situation if you want to let the application decide of the IP initialization method (you can set DHCP, AUTOIP, DHCP+AUTOIP)....

>MEMP_NUM_NETCONN should be at most the sum of MEMP_NUM_{TCP,RAW,UDP}_PCB


+MEMP_NUM_TCP_PCB_LISTEN?

>TCP_WND versus PBUF_POOL_SIZE*PBUF_POOL_BUFSIZE


Sure of that? TCP_WND isn't "per PCB", vs a "global" PBUF_POOL" is shared by all pcb ?


Frédéric Bernon <fbernon>
Group Member
Thu 02 Aug 2007 09:18:51 PM UTC, comment #1: 

Some more possibilites:

  • Error if !UDP && UDP_LITE
  • Error if !UDP && SNMP
  • Move deprecated options from opt.h into this file
  • Perhaps a warning on ETHARP_TCPIP_INPUT?
  • Perhaps do range checking on some values?
  • Perhaps a warning if DHCP && AUTOIP && !DHCP_AUTOIP_COOP


Jared Grubb <jgrubb>
Group Member
Thu 02 Aug 2007 07:52:12 PM UTC, original submission:  

There should be more sanity checking of configuration settings in lwipopts.h, for example:

- MEMP_NUM_TCP_SEG should be at least as big as TCP_SND_QUEUELEN
- MEMP_NUM_NETCONN should be at most the sum of MEMP_NUM_{TCP,RAW,UDP}_PCB
- MEMP_NUM_NETBUF should be at most the sum of MEMP_NUM_PBUF_POOL and MEMP_NUM_PBUF
- Ditto MEMP_NUM_ARP_QUEUE can be compared to the number of pbufs.
- Ditto IP_FRAG_USES_STATIC_BUF
- Ditto TCP_SND_QUEUELEN
- TCP_WND versus PBUF_POOL_SIZE*PBUF_POOL_BUFSIZE
- TCP_MSS <= TCP_WND
- TCP_SNDLOWAT < TCP_SND_BUF

There's probably more. We could consider ensuring that the number of pbufs exceed MEMP_NUM_NETBUF+MEMP_NUM_ARP_QUEUE+TCP_SND_QUEUELEN etc., but I don't think that's something to assert on because it's inherent in lwIP that you shouldn't assume the worst case, and instead try to recover if you run out of memory. But in other cases it's worth asserting when it shows the user doesn't really understand the dependencies.

Some can be checked at build time in the preprocessor, others may need to be done at runtime, but only in a debug build.

Jonathan Larmour <jifl>
Group Member

 

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

Attached Files
file #14004:  lwip.init.timeout.check.patch added by jifl (1KiB - text/x-patch)
file #13837:  lwip_sanity_check.patch added by fbernon (3KiB - text/x-patch)
file #13833:  lwip_sanity_check.patch added by fbernon (3KiB - 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 goldsimon (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by jgrubb (Posted a comment)
  • -email is unavailable- added by jifl (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 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-09-21 jifl Attached File- Added lwip.init.timeout.check.patch, #14004
    2007-09-07 fbernon StatusNone Done
        Percent Complete90% 100%
        Assigned toNone jifl
        Open/ClosedOpen Closed
    2007-09-07 fbernon Percent Complete70% 90%
    2007-08-30 jifl Percent Complete30% 70%
    2007-08-30 fbernon Percent Complete20% 30%
    2007-08-30 fbernon Attached File- Added lwip_sanity_check.patch, #13837
    2007-08-29 fbernon Attached File- Added lwip_sanity_check.patch, #13833
        Percent Complete0% 20%
    2007-08-29 kieranm Priority3 - Low 7 - High
        Planned ReleaseNone 1.3.0

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code