patchlwIP - A Lightweight TCP/IP stack - Patches: patch #9164, Add option to set and use peer DNS...

 
 

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

patch #9164: Add option to set and use peer DNS server

Submitter:  Ajay Bhargav <ajaybhargav>
Submitted:  Thu 17 Nov 2016 11:09:49 AM UTC
   
 
Category:  PPP Priority:  5 - Normal
Status:  Wont Do Privacy:  Public
Assigned to:  gradator Open/Closed:  Closed
Planned Release:  None

Jump to the original submission

Thu 17 Nov 2016 08:09:30 PM UTC, comment #8: 

Hi,


> I understand completely. For me a better way could be to have settings
> defined as a const pointer (save ram too) in case someone configure
> ppp parameters and do not want to change them later on.


I agree with you but lwIP users should not have to access struct members directly, it means generating a const configuration struct must be done by only using macros. I'm not saying it's not possible but this is going to be a pain.

About RAM usage, ppp_settings is 48 byte long with all features enabled with 64-bit alignment. ppp struct itself is more than 1000 byte long with minimal feature set (> 7000 byte long with all features enabled). I'm not sure it's worth it :)


> Most of the time there is only single ppp connection in an
> embedded device and settings can be assigned when pcb is
> allocated. This is where only compile time options are needed
> no runtime options.


I see the point, but this is not always true, for example you could have two identical systems communicating over PPPoS with a DIP switch to select which one is PPP server and which one is PPP client.


Sylvain

Sylvain Rochet <gradator>
Group Member
Thu 17 Nov 2016 02:44:30 PM UTC, comment #7: 

I understand completely. For me a better way could be to have settings defined as a const pointer (save ram too) in case someone configure ppp parameters and do not want to change them later on. Most of the time there is only single ppp connection in an embedded device and settings can be assigned when pcb is allocated. This is where only compile time options are needed no runtime options.

Ajay Bhargav <ajaybhargav>
Thu 17 Nov 2016 01:53:59 PM UTC, comment #6: 

Simon took the words right out of my mouth here :-)

To summarize, opt.h and opts.h are compile-time options, ppp_set macros and functions defined in ppp.h are runtime options, we try to prevent having an option in both in order to avoid confusion.

That is, if an option move from compile-time to runtime we will probably remove it from compile-time and add this behavior change in the upgrade documentation.

If necessary, we might add default values in ppp_opts.h if we need a runtime configuration with a value different than 0 as default and we will clearly state that it is a default value and not a compile-time option. I still prefer hardcoded default values if there is an available runtime config macro.

The changed DNS behavior from lwIP 1.4 to lwIP 2.0 is properly documented in PPP documentation in upgrade path section. There was actually no PPP DNS integration at all in lwIP 1.4, user had to do the gluing himself in the PPP state callback.

Basically, the balance between options that are compile-time options and as such change all PPP interfaces configuration in case of multiple PPP netif devices and options that are runtime options and as such are per PPP interface configuration is based on what users asked (and what I needed indeed ;) ) to be runtime configuration, everything else is compile-time. Therefore I am willing to move options from compile-time to runtime based on users input but not the contrary, which is mostly what Ajay is asking here; he actually asked for a default value change without affecting the runtime config but that was just a side effect instead of the primary intention.

Hope it clarified everything on this subject :-)

Sylvain

Sylvain Rochet <gradator>
Group Member
Thu 17 Nov 2016 12:56:13 PM UTC, comment #5: 

Sure.. we can drop this.

Ajay Bhargav <ajaybhargav>
Thu 17 Nov 2016 12:53:13 PM UTC, comment #4: 

We don't want more and more config options unless they have significant impact on code size or something else.

Your patch has no impact on code size but simply has the intention of configuring something via a define. That doesn't help much...

Simon Goldschmidt <goldsimon>
Group administrator
Thu 17 Nov 2016 12:50:49 PM UTC, comment #3: 

Ajay says, by email outside bug tracker:

> I guess, I missed it. I could not see it in ppp_opts.h so I thought
> it was missed. So shall we drop this or make something better if
> someone has a single PPP connection he/she can simply put this
> option in lwipopt?


Sylvain Rochet <gradator>
Group Member
Thu 17 Nov 2016 11:27:39 AM UTC, comment #2: 

I see most of the PPP settings are part of lwipopt.h so having this in the same place will make sense.

- Ajay

Ajay Bhargav <ajaybhargav>
Thu 17 Nov 2016 11:14:35 AM UTC, comment #1: 

Hello Ajay,

Did you miss "#define ppp_set_usepeerdns(ppp, boolval) (ppp->settings.usepeerdns = boolval)" in ppp.h ?

Sylvain

Sylvain Rochet <gradator>
Group Member
Thu 17 Nov 2016 11:09:49 AM UTC, original submission:  

usepeerdns setting of PPP is never set and there was no option to enable it either. This patch add a new PPP option "PPP_USEPEERDNS" which is enabled when LWIP_DNS is enabled and will set pcb->settings.usepeerdns setting in PPP pcb.

User can override this setting by defining PPP_USEPEERDNS option in
lwipopts.h file.

Ajay Bhargav <ajaybhargav>

 

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

Attached Files

 

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 dziegel (Updated the item)
  • -email is unavailable- added by gradator (Posted a comment)
  • -email is unavailable- added by ajaybhargav (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-11-17 goldsimon StatusNone Wont Do
        Open/ClosedOpen Closed
    2016-11-17 dziegel StatusNeed Info None
    2016-11-17 gradator StatusNone Need Info
        Assigned toNone gradator
    2016-11-17 ajaybhargav Attached File- Added 0001-ppp-Add-option-to-set-and-use-peer-DNS-server.patch, #39001

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code