Skip to content
This repository has been archived by the owner on Apr 24, 2020. It is now read-only.

[Bugfix] More consistent results from 'ip' on Non-OSX systems #1356

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jekotia
Copy link

@Jekotia Jekotia commented Aug 27, 2019

Description

Due to issues matching with some versions of iproute's 'ip' command, I've rewritten and thoroughly commented the non-OSX portion of p9k::parseIp. See #823

If desired, a lot of this can be rewritten to be shorter (and theoretically a smidge faster to execute); I chose to write the initial code in this manner to make it easier to read what I've done when reviewing the pull request.

Questions

None.

Additional notes against the bulletpoints from your PR guidelines:

  • This is against the master branch due to it being a bugfix
  • test/functions/utilities.spec has been run with no errors reported
  • No manual tests required AFAIK
  • No user configuration required
  • I don't believe this needs documentation in the wiki
  • I hope my commit message suffices

Due to issues matching with some versions of iproute's 'ip' command, I've rewritten and thoroughly commented. See Powerlevel9k#823

If desired, a lot of this can be rewritten to be shorter (and theoretically a smidge faster to execute); I chose to write the initial code in this manner to make it easier to read what I've done when judging the pull request.
@romkatv
Copy link

romkatv commented Aug 28, 2019

p9k::parseIp used to skip interfaces that aren't UP. Is this still the case with this PR?

@Jekotia
Copy link
Author

Jekotia commented Aug 28, 2019

Yes it is. Two checks are preformed; the first is dependent on POWERLEVEL9K_IP_INTERFACE being unset. If the value is unset, the check is made and it gets the name for the first interface which is UP.

The second check is preformed always; using either the interface name acquired in the first check or the interface name provided by the user configuration (POWERLEVEL9K_IP_INTERFACE), it acquires the IP address for that interface.

@romkatv
Copy link

romkatv commented Aug 28, 2019

I'll rephrase my question.

With the current implementation, if I set POWERLEVEL9K_IP_INTERFACE=foo, ip prompt segment won't be displayed unless network interface foo is UP (as can be seen in the output of ip link show foo).

Is this behavior preserved by the PR?

@Jekotia
Copy link
Author

Jekotia commented Aug 28, 2019

Yes it is.

@romkatv
Copy link

romkatv commented Aug 28, 2019

Yes it is.

Have you tried it? Once you get ip prompt segment working with POWERLEVEL9K_IP_INTERFACE=foo, type sudo ip link set foo down. This should hide ip segment.

@Jekotia
Copy link
Author

Jekotia commented Aug 29, 2019

Unfortunately, I do not have any systems to test this on (I use WSL on my computer, which it seems cannot toggle interfaces on the Windows host; the other *nix systems are servers and turning off an interface on any one of them would be followed by much yelling), but I see the problem you seem to be hinting at.

The way I've written this, if you manually specify an interface the IP address, if found, will ALWAYS be returned for that interface. The only instances where it will return nothing is if a) no interface is specified and no interface is up with an address b) a non-existent interface is specified.

I'll update this when I can.

@romkatv
Copy link

romkatv commented Aug 29, 2019

The way I've written this, if you manually specify an interface the IP address, if found, will ALWAYS be returned for that interface. The only instances where it will return nothing is if a) no interface is specified and no interface is up with an address b) a non-existent interface is specified.

That's right. The output of ip -o -f inet addr show foo is the same whether foo is UP or DOWN. Since it's the only command that your code calls when POWERLEVEL9K_IP_INTERFACE=foo, it's not possible to differentiate between UP and DOWN links.

Also note that the detection of link state is broken in the current code but in a different direction from your PR. While your PR will sometimes display an IP from an interface that is down, the current code will sometimes fail to display an IP from an interface that is up.

P.S.

It's a good idea to test your code on Linux and FreeBSD in addition to WSL. These systems have different network stacks and tools.

@Jekotia
Copy link
Author

Jekotia commented Aug 29, 2019

Regarding the issue in the current code, that's exactly what I'm trying (and less successfully than I had hoped) to address. It's failing to show the IP for an interface which is up, as explained in the referenced issue.

I've tested it on several CentOS and Debian systems, I just don't have the ability to flip their interfaces on and off since they're production systems at work.

@romkatv
Copy link

romkatv commented Aug 29, 2019

Regarding the issue in the current code, that's exactly what I'm trying (and less successfully than I had hoped) to address.

Yes, I've read your description of the issue affecting WSL. I'm referring to a different issue that affects Linux. I don't remember the details but the essential part is that the current code is mistakenly looking at the operational link state instead of its administrative state. Operational state is unreliable with many drivers and is best ignored.

I've tested it on several CentOS and Debian systems, I just don't have the ability to flip their interfaces on and off since they're production systems at work.

If you cannot get access to a physical Linux or FreeBSD box, use a VM. WSL is a poor substitute for Linux when it comes to testing your code because it has no Linux kernel. Linux in a VM on Windows will work just like a real Linux in all but the most obscure of corner cases. You could also try WSL2 or Docker, both of which run a real Linux kernel in a VM, but you'll still need access to FreeBSD or some other non-Apple BSD. Either way, it's a good investment of your time to learn how to run an OS in a VM.

P.S.

Please note that I'm neither a developer nor a user of Powerlevel9k. My opinion on what you should do comes with no authority whatsoever. Should you decide to heed or ignore my comments will have no bearing on your PR's acceptance chances.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants