patchlwIP - A Lightweight TCP/IP stack - Patches: patch #9876, PPP: Const-correctness fixes,...

 
 

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

patch #9876: PPP: Const-correctness fixes, pointer types for buffers & couple minor tweaks

Submitted by:  Freddie Chopin <freddie_chopin>
Submitted on:  Thu 05 Dec 2019 09:25:38 PM UTC  
 
Category:  PPP Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  Sylvain Rochet <gradator> Open/Closed:  Closed
Planned Release:  None

Tue 20 Oct 2020 03:51:15 AM UTC, comment #4: 

This is not such a big change after all. I have applied all patches, thank you Freddie !

Sylvain Rochet <gradator>
Project MemberIn charge of this item.
Sat 07 Dec 2019 12:14:50 PM UTC, comment #3: 

Hmm, I'm undecided. It's an API change, after all, and a rather cosmetic one... Maybe we can wait for the next "big" release for such a change.

The rest of the patches might still be valid?

Simon Goldschmidt <goldsimon>
Project Administrator
Fri 06 Dec 2019 07:07:33 AM UTC, comment #2: 

comment #1:

> void* and u8_t* is basically the same thing


Unless you are using C++, in which case they are not. When the API requires an u8_t* and you have a void*, then you have to use `static_cast<u8_t*>()`, otherwise the compiler produces an error.

> and adding some const hints to compiler. It does not fix any issue, does not add a feature


Unless you are using C++, in which case removing `const` has to be a separate step from converting the type and now your calls to lwIP APIs are starting to look a bit ridiculous:

pppos_input_tcpip(pcb_, static_cast<u8_t*>(const_cast<void*>(buffer)), size);

In my opinion the lack of `const` is a type error in PPP code, therefore I wouldn't call it "a hint" or "a cosmetic change". The "feature" this adds is relieving the user from having to cast everything all over the place. Note that this even allows removing a few casts from within PPP code, which were required only because PPP used different type (u8_t*) than the rest of lwIP (void* in payloads)

Final decision is obviously yours (;

Freddie Chopin <freddie_chopin>
Thu 05 Dec 2019 11:14:50 PM UTC, comment #1: 

Well… erm… I have mixed feelings about an API change just to change a pointer type (void* and u8_t* is basically the same thing) and adding some const hints to compiler. It does not fix any issue, does not add a feature, does not even prepare to welcome a new feature, in my opinion it is quite close to being just cosmetic and nitpick.

That would have been great before the 2.x release, now I'm not sure breaking something that works with something doing exactly the same thing but in a slightly different and incompatible way is worth it.

Sylvain

Sylvain Rochet <gradator>
Project MemberIn charge of this item.
Thu 05 Dec 2019 09:25:38 PM UTC, original submission:  

The major part of these patches is changing `u8_t*` to `const void*` for pppos_input_tcpip(), pppos_input() and pppos_output_cb_fn(). There is no good reason to use non-const buffers for these functions (and this is possibly dangerous - why should output callback modify what lwIP gives it or why should lwIP modify in-place what was given by user?).

While changes to pppos_input_tcpip() and pppos_input() are mostly harmless and should not affect any application, change to pppos_output_cb_fn() (which I actually consider the most important) is going to be non-backward compatible and breaking change in all possible cases. Unfortunately users will now have to change their prototypes (and maybe also slightly change the implementation). This is evident from the changes which I had to make in the example apps.

There is a similar problem for the functions from sio.h - while it may be debatable whether one should use `u8_t*` or `void*`, functions which do "writes" should use const pointers in all cases. Please let me know whether you're open to changing that.

Please compile-test before (hopefully) submitting, as I was probably not able to build every important combination of options and neither did I try to build the test apps.

Freddie Chopin <freddie_chopin>

 

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 gradator (Posted a comment)
  • -email is unavailable- added by freddie_chopin (Submitted the item)
  • -email is unavailable- added by freddie_chopin
  •  

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

    Date Changed by Updated Field Previous Value => Replaced by
    2020-10-20 gradator StatusNone => Done
        Assigned toNone => gradator
        Open/ClosedOpen => Closed
        SummaryConst-correctness fixes, pointer types for buffers & couple minor tweaks => PPP: Const-correctness fixes, pointer types for buffers & couple minor tweaks
    2019-12-05 freddie_chopin Attached File- => Added 0005-Remove-trailing-whitespaces-in-pppos_example.c.patch, #48010
        Attached File- => Added 0006-Remove-useless-cast-from-pppos_output_append-and-ppp.patch, #48011
    2019-12-05 freddie_chopin Attached File- => Added 0001-Use-const-void-instead-of-u8_t-in-pppos_input_tcpip.patch, #48006
        Attached File- => Added 0002-Use-const-void-instead-of-u8_t-in-pppos_input.patch, #48007
        Attached File- => Added 0003-Remove-useless-cast-from-pppos_input_sys.patch, #48008
        Attached File- => Added 0004-Use-const-void-instead-of-u8_t-in-pppos_output_cb_fn.patch, #48009
        Carbon-Copy- => Added gradator@gradator.net

    Back to the top


    Powered by Savane 3.5