patchlwIP - A Lightweight TCP/IP stack - Patches: patch #6459, Minor changes to err.h, stats.c...

 
 

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

patch #6459: Minor changes to err.h, stats.c & httpd/fs

Submitter:  Bill Auerbach <billauerbach>
Submitted:  Tue 11 Mar 2008 09:34:37 PM UTC
   
 
Category:  None Priority:  3 - Low
Status:  Done Privacy:  Public
Assigned to:  goldsimon Open/Closed:  Closed
Planned Release:  None

Jump to the original submission

Tue 17 Jun 2008 08:25:52 PM UTC, comment #10: 

Since I personally think overriding err_t to be s32_t for example is good on a 32-bit platform, I'll check the err_t overridable in and close this patch (since all other items have been solved or moved to patch #6483)

Simon Goldschmidt <goldsimon>
Group administrator
Sun 04 May 2008 07:51:12 PM UTC, comment #9: 

Except points for patch #6483, what is the status of others changes ? Should we close this record ?

Frédéric Bernon <fbernon>
Group Member
Sun 30 Mar 2008 08:53:43 AM UTC, comment #8: 

I have open patch #6483 "Stats module improvement" for that.

Frédéric Bernon <fbernon>
Group Member
Sun 30 Mar 2008 08:12:36 AM UTC, comment #7: 


>Apart from that, I think it's sad IGMP has an extra function - it is a protocol, too, after all!


yes, but we can't reduce others the protocol statistics to the stats_proto (by example, there is no fields in this struct to get informations or join/leave/reports, etc...).

>In other words, I think it would be good to add a function stats_display_proto(struct stats_proto) or something to selectively display the stats of one protocol only instead of displaying all. Only the current functions are not nice enough for that.


yes, I think we could have some defines like ...

#define STATSDISPLAY_LINK   0x00000001
#define STATSDISPLAY_ETHARP 0x00000002
#define STATSDISPLAY_IPFRAG 0x00000004
...

add change the stats_display prototype with a "u32_t flags" parameter, or add a stats_display_with_flags function and make stats_display as a macro

>Keeping the a const char name for all protocols could help here.


yes, but you have to cast the first parameters.

But I think it's another task, and we should "remove" stats from the scope of this patch...

Frédéric Bernon <fbernon>
Group Member
Thu 27 Mar 2008 09:46:03 PM UTC, comment #6: 

If we export those functions, we should find a way that the 'char name' is not needed (in stats_display_proto).

Apart from that, I think it's sad IGMP has an extra function - it is a protocol, too, after all!

In other words, I think it would be good to add a function stats_display_proto(struct stats_proto) or something to selectively display the stats of one protocol only instead of displaying all. Only the current functions are not nice enough for that. Keeping the a const char name for all protocols could help here.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 27 Mar 2008 06:58:28 PM UTC, comment #5: 

(Jim Pettinato wrote)

I exported stats_display to my telnet module to be able to read the stats over the network, so making them static would break that. I vote to continue to conditionally include the functions and keep them exportable - for at least the top-level stats_display() function (and maintain the function prototypes with the same conditional include in stats.h).

Frédéric Bernon <fbernon>
Group Member
Thu 27 Mar 2008 06:57:29 PM UTC, comment #4: 


>stats.h/.c: I'd vote for making the functions static. Frédéric, which function do you use externally? And how do you do it right now?


Actually, this is mainly stats_display_igmp. I have to declare it as "extern void stats_display_igmp(struct stats_igmp *igmp);", and I call "stats_display_igmp(&lwip_stats.igmp);". But if you change it to static, this will give a linker error.

But I think the problem is more to have the possibility to select which stats I want to display (that's why I don't use stats_display, because it's too verbose). Perhaps add a parameter to stats_display to select which stats to print could be a solution.


Frédéric Bernon <fbernon>
Group Member
Thu 27 Mar 2008 06:43:39 PM UTC, comment #3: 

httpd: checked in const filenames and u32_t file sizes (that's a real bug: no check was done anywhere while converting the original file size from int to u16_t...)

err_t: I think it's a good idea: This has always been a size tradeoff which makes lwIP slow on 32-bit processors. But I don't know which is the best thing to do. Using enums is not that cool in my opinion: On some systems, you might want to force it to 8bit... (Seeing error-names in the debugger would be nice, though.)

I'd make it overridable for a start, but I think that's a thing to discuss...

stats.h/.c: I'd vote for making the functions static. Frédéric, which function do you use externally? And how do you do it right now?

Simon Goldschmidt <goldsimon>
Group administrator
Sun 16 Mar 2008 08:21:48 AM UTC, comment #2: 

(Bill's answer from mailing list)

>
> Follow-up Comment #1, patch #6459 (project lwip):
>
> I would prefer to keep available stats functions to be able to call in my
> application.


I accept this.  Then can we please put the prototypes for them in stats.h?
Because there are no prototypes so I assumed they couldn't (wouldn't) be
called outside of stats.c.  (And my compiler is picky and warns on no
prototypes on public functions which is how I found this.)

> For err_t, can you explain why you need that?


To use the native type for a processor which on 16/32 bit processors can be
faster than chars.  Also, signed chars on many 8-bit processors are
expensive (e.g. Z80 and family need several instructions to sign extend and
also need runtime calls to compare signed chars).  If we could use sizeof in
#if's we could set the type to the native processor size automatically.  Or
use an enum for errors which makes debugging more meaningful and enums are
the native size of the processor.

I can build each way and see if there is a code size change for me, but it's
one platform test.

It's not urgent, but overriding the type and seeing the comment to be able
to might lead someone to change it for better performance and/or code size.
Admittedly, the gain is probably very minor.

> For httpd, to be honest, I don't use it, and I didn't think that anyone
> use it in production. So, no objections for that...


Ok, then maybe we don't need it.  I used it as a base for a simple WEB
server and was sending a JPG over 64k.  Pointers to the file system (which
in this example is stored in ROM) are better to have const.

Bill

Frédéric Bernon <fbernon>
Group Member
Thu 13 Mar 2008 08:08:31 PM UTC, comment #1: 

I would prefer to keep available stats functions to be able to call in my application.

For err_t, can you explain why you need that?

For httpd, to be honest, I don't use it, and I didn't think that anyone use it in production. So, no objections for that...

Frédéric Bernon <fbernon>
Group Member
Tue 11 Mar 2008 09:34:37 PM UTC, original submission:  

err.h - Allow overriding err_t type.

stats.c - Make 3 functions static not used outside of stats.c

httpd.c - Add const to file in http_state.
          Use u322_t to support files > 64.

fs.c / fs.h - Add const to name in fs_open since it could point to ROM.

Bill Auerbach <billauerbach>

 

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

Attached Files
file #15236:  err_h.diff added by billauerbach (531B - application/octet-stream)
file #15237:  stats_c.diff added by billauerbach (1017B - application/octet-stream)
file #15238:  httpd.diff added by billauerbach (2KiB - 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 fbernon (Posted a comment)
  • -email is unavailable- added by billauerbach (Submitted the item)
  • -email is unavailable- added by billauerbach
  •  

    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 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-06-17 goldsimon StatusNone Done
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2008-03-11 billauerbach Attached File- Added err_h.diff, #15236
        Attached File- Added stats_c.diff, #15237
        Attached File- Added httpd.diff, #15238
        Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code