Skip to content

Add Meter Choice framework#200

Draft
cgzones wants to merge 8 commits intohtop-dev:mainfrom
cgzones:ip
Draft

Add Meter Choice framework#200
cgzones wants to merge 8 commits intohtop-dev:mainfrom
cgzones:ip

Conversation

@cgzones
Copy link
Copy Markdown
Member

@cgzones cgzones commented Oct 1, 2020

Add the possibility for meters to have selectable choices, so they can display more exact information the user is interested in.

As examples the following meters are added:

  • Hostname: show the IP address based on the selected network interface
  • DiskUsage: show the space utilization based on the selected mountpoint
  • NetworkIO: show the IO usage based on the selected network interface
  • LibSensorsFan: show the speed based on the selected libsensors provided sensor
  • LibSensorsTemp: show the temperature based on the selected libsensors provided sensor

Data is only available on Linux for now.

@cgzones cgzones added the RFE label Oct 1, 2020
@cgzones cgzones force-pushed the ip branch 3 times, most recently from 1571ee6 to 931f16e Compare October 1, 2020 13:01
@BenBE
Copy link
Copy Markdown
Member

BenBE commented Oct 1, 2020

Not tested, but does this list all local IP addresses, or does it select one? Like: Waht happens on a multihomed system with e.g. LAN+wireless+VPN?

@cgzones
Copy link
Copy Markdown
Member Author

cgzones commented Oct 1, 2020

Only the the first (whatever the kernel decides to be first) non-loopback link-local IP address is displayed.

Maybe in the future Meters can have runtime options, which can be set per enabled-meter (like display-mode).
Than one can select which IP to display.

@BenBE
Copy link
Copy Markdown
Member

BenBE commented Oct 1, 2020

$ ip a s
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000
    inet 127.0.0.1/8 scope host lo
    inet6 ::1/128 scope host 
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP group default qlen 1000
    inet 10.116.112.196/24 brd 10.116.112.255 scope global dynamic noprefixroute eth0
    inet6 fe80::fedc:ba98:7654:3210/64 scope link noprefixroute 
3: wlan0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default qlen 1000
    inet 10.116.128.196/17 brd 10.116.255.255 scope global dynamic noprefixroute wlan0
    inet6 2a07:1:9:6:7:7:5:e/64 scope global temporary dynamic 
    inet6 2a07:1:9:6:e:0:a:f/64 scope global dynamic mngtmpaddr noprefixroute 
    inet6 fc00::4242:9:a:1:a/64 scope global temporary dynamic 
    inet6 fc00::4242:8:7:e:4/64 scope global dynamic mngtmpaddr noprefixroute 
    inet6 fe80::e:a:6:2/64 scope link noprefixroute 
4: br-lxc: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default qlen 1000
    inet 192.168.254.1/24 brd 192.168.254.255 scope global br-lxc
    inet6 fe80::c000:8000:4000:23/64 scope link 
5: virbr0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc noqueue state DOWN group default qlen 1000
    inet 192.168.122.1/24 brd 192.168.122.255 scope global virbr0
7: lxcbr0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc noqueue state DOWN group default qlen 1000
    inet 10.0.3.1/24 scope global lxcbr0

$ ip r s
default via 10.116.112.254 dev eth0 proto dhcp metric 100 
default via 10.116.128.6 dev wlan0 proto dhcp metric 600 
10.0.3.0/24 dev lxcbr0 proto kernel scope link src 10.0.3.1 linkdown 
10.116.112.0/24 dev eth0 proto kernel scope link src 10.116.112.196 metric 100 
10.116.128.0/17 dev wlan0 proto kernel scope link src 10.116.128.196 metric 600 
192.168.122.0/24 dev virbr0 proto kernel scope link src 192.168.122.1 linkdown 
192.168.254.0/24 dev br-lxc proto kernel scope link src 192.168.254.1 

$ ip -6 r s
::1 dev lo proto kernel metric 256 pref medium
2a07:1:9:6::/64 dev wlan0 proto ra metric 600 pref medium
fc00:0:0:4242::/64 dev wlan0 proto ra metric 600 pref medium
fe80::/64 dev eth0 proto kernel metric 100 pref medium
fe80::/64 dev br-lxc proto kernel metric 256 pref medium
fe80::/64 dev wlan0 proto kernel metric 600 pref medium
default via fe80::cafe:beef:beef:1 dev wlan0 proto ra metric 600 pref medium

Expected behaviour:
IPv4: 10.116.112.196 (Default with lowest metric)
IPv6: 2a07:1:9:6:e:0:a:f (Routable global network address on default-routed interface)

While fc00::4242:8:7:e:4 would be correct too, it's an ULA, and thus not globally routable.

For the difference between dynamic mngtmpaddr noprefixroute and temporary dynamic see the docs ;-)

@ghost
Copy link
Copy Markdown

ghost commented Oct 14, 2020

Congratulations 🎉. DeepCode analyzed your code in 2.417 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

Copy link
Copy Markdown
Member

@BenBE BenBE left a comment

Choose a reason for hiding this comment

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

General issue with IPv6 is if the interface has multiple temporary+dynamic addresses than you might end up showing deprecated addresses or addresses only intended for outgoing connections. E.g. right now my wifi interface shows 1 address scoped link, 6 scoped global, of which 5 are temporary dynamic (4 of which deprecated). The last one is dynamic mngtmpaddr noprefixroute … If only one is shown it should most likely be that one.

Settings.c Outdated
Comment on lines +229 to +496
} else if (String_eq(option[0], "left_meter_choices")) {
Settings_readMeterChoices(this, option[1], 0);
didReadMeters = true;
} else if (String_eq(option[0], "right_meter_choices")) {
Settings_readMeterChoices(this, option[1], 1);
didReadMeters = true;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does it cope with just reading Meter Choices, but no associated/non-matching meters?
Does it properly auto-upgrade from existing config?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i hope; i tested several cases

@BenBE BenBE added the feature request Completely new feature requested label Mar 23, 2021
@cgzones cgzones force-pushed the ip branch 4 times, most recently from 07e5760 to ec88ab7 Compare May 2, 2021 15:21
@cgzones cgzones force-pushed the ip branch 3 times, most recently from 2b13f33 to 8120b82 Compare June 3, 2021 13:43
@cgzones cgzones changed the title Add Hostname meters with local IP address Add Meter Choice framework Jun 3, 2021
@cgzones cgzones marked this pull request as ready for review June 3, 2021 16:16
Use a meter attached data structure instead of static variables, to
uncouple different meters for choice support.
Display the network usage based on the selected network interface.

Currently only implemented on Linux and FreeBSD.
Show the input and maximum temperature of any available libsensors
temperature sensor.

Related: htop-dev#610
Show the input, minimum and maximum fan speed of any available
libsensors fan sensor.
}
}

out:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
out:
out:

}
}

out:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
out:
out:

Comment on lines +1147 to +1148
unsigned int ifs = ifconf.ifc_len / sizeof(*ifr);
for (unsigned int i = 0; i < ifs; i++) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
unsigned int ifs = ifconf.ifc_len / sizeof(*ifr);
for (unsigned int i = 0; i < ifs; i++) {
size_t ifs = ifconf.ifc_len / sizeof(*ifr);
for (size_t i = 0; i < ifs; i++) {


struct in6_addr s6;
char ifName[16];
unsigned int count = 0;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
unsigned int count = 0;
size_t count = 0;

@BenBE
Copy link
Copy Markdown
Member

BenBE commented Aug 22, 2022

Testing the Disk Usage meter the UX is not the best. Especially as it does not filter /snap, /run and several other memory FS. Furthermore the UI hangs if the filesystem is not reachable (e.g. with CIFS).

For the Hostname meter I'd like to have a combined Host IPv4/IPv6 meter to reduce information duplication. Or just have a blank IP address meter.

smalinux added a commit to smalinux/htop that referenced this pull request Sep 11, 2022
htop-dev#669)

Signed-off-by: Sohaib Mohamed <sohaib.amhmd@gmail.com>
smalinux added a commit to smalinux/htop that referenced this pull request Sep 11, 2022
…v#669)

Signed-off-by: Sohaib Mohamed <sohaib.amhmd@gmail.com>
smalinux added a commit to smalinux/htop that referenced this pull request Sep 11, 2022
…v#669)

Signed-off-by: Sohaib Mohamed <sohaib.amhmd@gmail.com>
smalinux added a commit to smalinux/htop that referenced this pull request Sep 11, 2022
…v#669)

Signed-off-by: Sohaib Mohamed <sohaib.amhmd@gmail.com>
smalinux added a commit to smalinux/htop that referenced this pull request Sep 11, 2022
…v#669)

Signed-off-by: Sohaib Mohamed <sohaib.amhmd@gmail.com>
smalinux added a commit to smalinux/htop that referenced this pull request Sep 11, 2022
…v#669)

Signed-off-by: Sohaib Mohamed <sohaib.amhmd@gmail.com>
smalinux added a commit to smalinux/htop that referenced this pull request Sep 12, 2022
Related: htop-dev#200 && htop-dev#669

Signed-off-by: Sohaib Mohamed <sohaib.amhmd@gmail.com>
smalinux added a commit to smalinux/htop that referenced this pull request Sep 12, 2022
Related: htop-dev#200 && htop-dev#669

Signed-off-by: Sohaib Mohamed <sohaib.amhmd@gmail.com>
smalinux added a commit to smalinux/htop that referenced this pull request Sep 12, 2022
Related: htop-dev#200 && htop-dev#669

Signed-off-by: Sohaib Mohamed <sohaib.amhmd@gmail.com>
smalinux added a commit to smalinux/htop that referenced this pull request Sep 12, 2022
Related: htop-dev#200 && htop-dev#669

Signed-off-by: Sohaib Mohamed <sohaib.amhmd@gmail.com>
@BenBE
Copy link
Copy Markdown
Member

BenBE commented Sep 12, 2022

Wondering if choices should have a preferred sorting order (either enforced by the "choice provider" or by the "meter framework".

Copy link
Copy Markdown
Member

@natoscott natoscott left a comment

Choose a reason for hiding this comment

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

@cgzones strictly, DiskUsage file names and associated code function/variable naming should be FilesystemUsage ... can we rename this?

smalinux added a commit to smalinux/htop that referenced this pull request Sep 13, 2022
Related: htop-dev#200 && htop-dev#669

Signed-off-by: Sohaib Mohamed <sohaib.amhmd@gmail.com>
smalinux added a commit to smalinux/htop that referenced this pull request Sep 15, 2022
Related: htop-dev#200 && htop-dev#669

Signed-off-by: Sohaib Mohamed <sohaib.amhmd@gmail.com>
smalinux added a commit to smalinux/htop that referenced this pull request Sep 15, 2022
Related: htop-dev#200 && htop-dev#669

Signed-off-by: Sohaib Mohamed <sohaib.amhmd@gmail.com>
@BenBE BenBE modified the milestones: 3.3.0, 3.4.0 Dec 26, 2023
@cgzones cgzones removed this from the 3.4.0 milestone Apr 13, 2024
@cgzones cgzones marked this pull request as draft April 13, 2024 19:40
@ChillarAnand
Copy link
Copy Markdown

Do you have any plans on merging this? I am new to this code base, but can push minor changes/features.

@BenBE
Copy link
Copy Markdown
Member

BenBE commented Jul 15, 2024

There are still 3 major issues with this PR, that need to be resolved, before it can be merged:

  1. Rebase to current main to resolve merge conflicts
  2. Find a better solution to the UI side of things for choosing the item from the available list of options
  3. Test things to work properly and stable

While item 1 can be resolved quite easily, it's mostly item 2 on the list, that still needs experimentation. If you happen to have some ideas that are intuitive, comfortable to use and easily discoverable we'd be glad to hear about them. The current solution in this PR horribly fails on the "comfortable to use" side of things …

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature request Completely new feature requested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants