search

mirage / charrua

A DHCP library in OCaml
http://mirage.github.io/charrua/
ISC License
55 stars 18 forks source link

Allow optional argument to be erased in Dhcp_server.make. #111

Closed haesbaert closed 3 years ago

haesbaert commented 3 years ago

Changes public API, might will break users.

haesbaert commented 3 years ago

Don't merge yet, I want to run it for a week at home with a very small lease time, I'm trying to force the corner cases.

hannesm commented 3 years ago

thanks, this all looks good (but I haven't properly reviewed the code). especially the lease database rewrite is highly welcome, it looks simpler now :)

haesbaert commented 3 years ago

Been running this for over a week at home with very short lease times, I think all the bugs I was hunting were gone. I've also added some sugar to the unix binary.

This is good to go imho, if someone could review, would be noice :).

The bigger change is that the caller should call Dhcp_server.garbage_collect on the db once in a while, (once a day would be probably enough).

hannesm commented 3 years ago

Thanks for your testing and improvements. This looks fine, the maps for leases are, as far as I understand, needed since on the one hand we need to be able to receive the lease based on the IPv4 address, and based on the client_id (thus we need two maps with two different keys). This all makes sense, and there's care that the maps are kept in sync (see add/remove/..). :)

I'd be fine merging this (maybe take a look what the lint of the CI would like to improve to get a green checkmark).

haesbaert commented 3 years ago

Oh I want to remove the sanity_check from replace, I kept there to make sure it was correct, which at this point I think it is. I'd keep the call on garbage_collect still.

On Thu, 15 Jul 2021 at 12:15, Hannes Mehnert @.***> wrote:

Thanks for your testing and improvements. This looks fine, the maps for leases are, as far as I understand, needed since on the one hand we need to be able to receive the lease based on the IPv4 address, and based on the client_id (thus we need two maps with two different keys). This all makes sense, and there's care that the maps are kept in sync (see add/remove/..). :)

I'd be fine merging this (maybe take a look what the lint of the CI would like to improve to get a green checkmark).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mirage/charrua/pull/111#issuecomment-880574930, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABR2EA2Z3TJLKIOJNVEUJLTX2YLVANCNFSM4722K7NQ .

hannesm commented 3 years ago

Oh I want to remove the sanity_check from replace

maybe do this before a merge & release? I like released code without assertions ;)

haesbaert commented 3 years ago

I think this should be good to go now, I removed the sanity_check from replace.

hannesm commented 3 years ago

thanks, merged.