added weak fnc add_custom_offer_options

[bwjohns4]

Here is an attempt to incorporate add_custom_offer_options(). This is a weak function that is called at the end of add_offer_options() that allows another library to redefine the function to hook into and add custom DHCP options.

One question I have is what restrains the size of optptr? How much can it grow (via *optprt++) before causing problems? Is it in an allocated space already?

[bwjohns4]

I'm not sure what/how to do anything with the workflow. Please let me know if I need to do anything there.

[earlephilhower]

@bwjohns4 the CI failed due to indent/spacing issues. Run tests/restyle.sh and commit/push the changes and it should clear up.

[bwjohns4]

@earlephilhower , do I just run that script from the root of the repo directory? I ran it there and nothing appears to have changed to the file. Do I need to pass any arguments into the script? Here are the error messages that I get:

[earlephilhower]

Your screenshot looks like Ubuntu terminal. sudo apt install astyle or equivalent will be required to get the style tool installed, then you can re-run the script to do the fixes.

[d-a-v]

One question I have is what restrains the size of optptr? How much can it grow (via *optprt++) before causing problems? Is it in an allocated space already?

Arduino/cores/esp8266/LwipDhcpServer.cpp

Line 72 in 9fcf14f

uint8_t options[312];

Arduino/cores/esp8266/LwipDhcpServer.cpp

Line 532 in 9fcf14f

end = add_msg_type(&m->options[4], DHCPOFFER);

This code is an as-is-copy of the original espressif dhcp server from the initial lwip-1.4.
As we can see there's not much control.
After a second look, what about holding a trivial linked list of callbacks that would be callable from ::send_offer() ?
These callbacks would pass the option pointer and the remaining available size (end - &m->options[4]).

[bwjohns4]

Where in Arduino is the DCHP server instantiated and where in user/library code would this happen? What about adding a setter to the DHCPserver class to pass in custom options as well as a bool pointer that can be used to pass back true or false success of adding in the custom options.

Then when it's time to add the custom options just check that end - &m->options[0] is less than 312 - 1 (subtract one for the 255 byte appended to the end). If it's too big then fail and pass the status back to the user/library. I don't think I'm 100% clear on the best way to communicate the success/fail back to the user/lib.

How does this align with your suggestion of the linked list callable from send_offer()

[d-a-v]

Where in Arduino is the DCHP server instantiated and where in user/library code would this happen?

It is globally instantiated. Nonosdk firmware initially calls dhcps_start() which is defined in the same file.

[...] How does this align with your suggestion of the linked list callable from send_offer()

send_offer() is called much later than when dhcp server is initialized, and that, on user request after explicitly installing AP mode.
So user sketch has time to set something up to be taken into account.
I was mentioning a linked list. A simple mechanism may be sufficient:
In the dhcp server class, a std::function<bool(uint8_t* option, size_t remain)> member, and a setter.

This API can be overloaded if one need to separately add more options, which I doubt.

[bwjohns4]

What would be the use case for the linked list here? Build a linked list of options and their associated parameters to add?

[d-a-v]

What would be the use case for the linked list here? Build a linked list of options and their associated parameters to add?

No. It was in case dhcp options would have to be added more than once and from different locations in user code.
This is unlikely and I shouldn't have been mentioning it.

Lets keep it simple. I think your PR will be fine if you add a size parameter to the weak function.

edit after #8571 review

Size is the first byte of user data pointer.
Current code needs to be updated to pass whatever is needed to check with the option buffer size.

[d-a-v]

Thanks
This probably should be merged before #8546

[mcspr]

...it would make sense to re-do this as a class method though?
plus, the suggested improvement of fixed-size buffer and some kind of limit

[bwjohns4]

So I've got a draft after redoing this to use class methods and move away from the weak function idea. I'll do some testing and push that here asap.