Skip to content

Conversation

@benjamreis
Copy link
Contributor

The link local addresses can't be used

@benjamreis benjamreis force-pushed the filter-link-local-address branch from 604196d to 29c7444 Compare June 22, 2023 13:09
The link local addresses can't be used

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>
@benjamreis benjamreis force-pushed the filter-link-local-address branch from 29c7444 to 5328696 Compare June 22, 2023 13:32
Copy link
Contributor

@lindig lindig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this filter should be factored out into one of the helper modules and not repeated. This could be a (sub) module that has IPv6 related functions. In general I would like to see the use of an abstract address type that internally represents IPv6 and IPv4 rather than strings. That would be a bigger change but we should avoid having ad-hoc string-based solutions proliferating.

@benjamreis
Copy link
Contributor Author

So if I mutualize and use this lib: https://opam.ocaml.org/packages/ipaddr/

Would that be okay?
We can then extend bit by bit the use of the lib in other cases.

@lindig
Copy link
Contributor

lindig commented Jul 18, 2023

Yes, using such a library to encapsulate the address would be a good step. I don't know how feasible that is, though.

@psafont
Copy link
Member

psafont commented Aug 2, 2023

I believe this won't be merged as-is. It's better to open a new one once the ipaddr library is used. Note that the ipaddr version used has bugs with certain ranges of IPv6 addresses. I'm working to include a fixed version.

@psafont psafont closed this Aug 2, 2023
@benjamreis
Copy link
Contributor Author

New version using the Ipaddr lib as asked here: #5463

@benjamreis benjamreis deleted the filter-link-local-address branch February 21, 2024 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants