Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display IPv6 addresses using condensed notation #850

Closed
darkain opened this Issue Sep 14, 2018 · 11 comments

Comments

Projects
None yet
3 participants
@darkain
Copy link

darkain commented Sep 14, 2018

Right now IPv6 addressed are displayed using their long-form notation, which takes up needless additional space on smaller console windows.

Example:

root@router-1:~ # zerotier-cli listpeers
...
200 listpeers e4da7455b2 2001:41d0:0001:4982:0000:0000:0000:0001/52203;7653;7653 3809 1.2.5 LEAF

That address should be condensed down to 2001:41d0:1:4982::1 - a 20 character reduction, making it fit on a 80 character width terminal.

Also, yes, I'm aware that not all addresses can be condensed, but this is common enough that it helps readability.

@glimberg

This comment has been minimized.

Copy link
Member

glimberg commented Sep 14, 2018

We happily accept pull requests on the dev branch

@darkain

This comment has been minimized.

Copy link
Author

darkain commented Sep 14, 2018

When I get some time, I'll look into this. Right now, still working on getting various FreeBSD/ARM builds going!

@darkain

This comment has been minimized.

Copy link
Author

darkain commented Sep 14, 2018

Just glancing over the IPv6 string handling routines, I've found some bugs in them too where it'll parse some addresses wrong. Once I'm home from work tonight and on my free time, I'll put together a patch that handles both generating and parsing shorthand IPv6 notation.

@darkain

This comment has been minimized.

Copy link
Author

darkain commented Sep 15, 2018

Is there a particular reason why the arpa/inet functions are not used for translating between binary and string representation of IPv4 and IPv6 addresses, and instead ZT has its own rolled in?

In the current ZT implementation when I was looking into properly shortening IPv6 string representations, I also looked at how ZT parses IPv6 strings. The implementation in question will incorrectly parse shortened IPv6 addresses, which could cause problems.

I have a patch for this already, and am currently testing it locally on various platforms.

@adamierymenko

This comment has been minimized.

Copy link
Contributor

adamierymenko commented Sep 17, 2018

Initially it was done for portability and for small devices. We could use them but haven't needed to change.

@adamierymenko

This comment has been minimized.

Copy link
Contributor

adamierymenko commented Sep 17, 2018

There was a goal of keeping any OS/platform specific functions out of the core.

@adamierymenko

This comment has been minimized.

Copy link
Contributor

adamierymenko commented Sep 17, 2018

The core in particular is extremely dependency-phobic.

@darkain

This comment has been minimized.

Copy link
Author

darkain commented Sep 17, 2018

The reason I ask is because there is some bugs in your implementation. Also, the required dependencies are already part of your core, as seen here: https://github.com/zerotier/ZeroTierOne/blob/master/include/ZeroTierOne.h#L43

inet functions are pretty standard across the board in all OSes and IP implementations. Your code is already using the AF_INET/AF_INET6 defines, just not the inet_ functions.

Would you be opposed to a PR to switch these string functions over to use the provided standard ones?

@adamierymenko

This comment has been minimized.

Copy link
Contributor

adamierymenko commented Sep 17, 2018

Go for it. We can always use our own if we need to port to some kind of tiny embedded device without these functions.

@darkain

This comment has been minimized.

Copy link
Author

darkain commented Sep 22, 2018

Whoops, forgot to note it here: PR #853

@adamierymenko

This comment has been minimized.

Copy link
Contributor

adamierymenko commented Sep 25, 2018

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.