Ethernet: Fix out of bounds memset on reset DHCP lease

[scop]

No description provided.

[matthijskooijman]

Looking at the comment, this memset is intentionally out-of-bounds, so it doesn't just reset the localIP variable, but also the subsequent variables. This is of course fairly fragile (and confusing), so a better approach might be to store all lease-related variables that need to be reset into a struct and then memset the struct instead of separate variables.

[scop]

Oh, confusing indeed. Anything in particular wrong with memsetting the individual ones?

[matthijskooijman]

Performance (time and code space) I suspect, but that's not really a compelling reason for something that happens as rarely as this. So splitting in different memsets would indeed be a good approach. Care to try and see how much extra flash that takes? Perhaps gcc surprises us and joins them together? :-)

[cousteaulecommandant]

Maybe a more self-explanatory code would be a good idea, like

memset(&_dhcpLocalIp, 0, (char*)(&_dhcpLeaseTime) - (char*)(&_dhcpLocalIp));

or at least a comment clarifying that 5 members are being cleared at once.
All this after testing whether or not GCC optimizes this, of course; I'd think it should be smart enough to know what memset does.

[Rotzbua]

Ethernet has a extra repo, see: https://github.com/arduino-libraries/Ethernet

I reported this already: arduino-libraries/Ethernet#45

Memset should be used for every variable, the actual code is a nightmare.

[matthijskooijman]

Hm, then a pullrequest should probably be based on the external repo then.
@facchinm, @cmaglie, what's the policy for this? Isn't the idea to remove a library from this repo when a separate one is created for it?

[facchinm]

@matthijskooijman the idea is to remove the library from the main repo once it has no PR on it (or they have been moved to the new repo). Ironically, it was easier to move SAM folder than Ethernet library 😄

[matthijskooijman]

@facchinm, but now that a separate repo exists and the library is still included in this repo, any merged PRs in this repo will have to be manually ported over to the separate repo, right (and vice versa as well)? Wouldn't it make sense to just remove the library from this repo? You would need to manually port the PRs over to the other repo, but you need to do that anyway. And it prevents new PRs from appearing here. Or am I missing something here?

[facchinm]

No, you are not missing anything 😄 We only need to allocate some time to move everything to the new repo and then we can simply delete the folder here.

[matthijskooijman]

@facchinm, but now that a separate repo exists and the library is still included in this repo, any merged PRs in this repo will have to be manually ported over to the separate repo, right (and vice versa as well)? Wouldn't it make sense to just remove the library from this repo? You would need to manually port the PRs over to the other repo, but you need to do that anyway. And it prevents new PRs from appearing here. Or am I missing something here?