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

[#480] SysarchMeter to view kernel version/arch info #493

Merged
merged 1 commit into from
Jan 31, 2021
Merged

[#480] SysarchMeter to view kernel version/arch info #493

merged 1 commit into from
Jan 31, 2021

Conversation

ahgamut
Copy link
Contributor

@ahgamut ahgamut commented Jan 27, 2021

SysarchMeter calls the uname function to obtain the kernel version and
architecture in a utsname variable. If the version data of the utsname is not empty, the distro version is also printed.

SysarchMeter follows the style of

  • HostnameMeter (uses the same attributes) and
  • DiskIOMeter (uses snprintf to print to buffer).

Used astyle -r -xb -s3 -p -xg -c -k1 -W1 \*.c \*.h to format the two newly added files.

Sample Screenshot:
Screenshot from 2021-01-27 15-27-00

@ahgamut
Copy link
Contributor Author

ahgamut commented Jan 27, 2021

I've updated all the Platform.c files except for unsupported/Platform.c. Should I change it for that as well?

The man page for uname says that uname conforms to:

POSIX.1-2001, POSIX.1-2008, SVr4.  There is no uname() call in 4.3BSD.

Does that affect any of the Platform.c files updated in these commits?

@BenBE
Copy link
Member

BenBE commented Jan 27, 2021

Thank you for writing this PR. On first glance the code looks good and reasonable with a few details to still polish. As I'm a bit short on time right now, just a rough overview of what I saw; will take another look later.

  • The copyright header has the wrong year. ;-) Even if you could argue, that we somehow entered an infinite 2020 at some point in March … :P
  • After the last #include there are usually two blank lines before the actual code part of the file.
  • In your output of the meter have a closer look at the original example again. For your system the output of the meter should probably end in / Debian 10 [buster]. Cf. lsb_release for more details. I hope this also explains the note "skip this part if not available" in the original description.
  • I wonder if it should be SysArchMeter? (up-case A)

Feel free to squash and amend your commits on your branch as needed. Having things in as few independent commits as reasonable, without loosing logical separation of steps is much preferred.

@BenBE
Copy link
Member

BenBE commented Jan 27, 2021

I've updated all the Platform.c files except for unsupported/Platform.c. Should I change it for that as well?

Sure.

Just feel free to implement the meter for the "unsupported" platform by hard-coding something like Unknown Platform with a comment near it reminding the developer to replace this with an implementation based on e.g. uname/lsb_release (or similar for that platform) as required for the actual platform. The "Unknown" platform is more or less a template for people who want to port htop to some new architecture/OS.

The man page for uname says that uname conforms to:

POSIX.1-2001, POSIX.1-2008, SVr4.  There is no uname() call in 4.3BSD.

Does that affect any of the Platform.c files updated in these commits?

Nope. For all practical purposes we can assume uname exists on all sensible, supported platforms.

FWIW: I'll happily review a PR with a minimal patch to make this work by the first person to demonstrate via video how he got the code compiling on plain 4.3BSD with an C99 compliant compiler … ;-)

@ahgamut
Copy link
Contributor Author

ahgamut commented Jan 27, 2021

The copyright header has the wrong year. ;-)   
Even if you could argue, that we somehow entered an infinite 2020 at some point in March … :P

I'm still stuck in 2020 xD, but I changed the files to have 2021.

After the last `#include` there are usually two blank lines before the actual code part of the file.

HostnameMeter.h has only one line after the last #include, so I just followed that.

In your output of the meter have a closer look at the original example again. For your system the output of the meter should probably end in / Debian 10 [buster]. Cf. lsb_release for more details. I hope this also explains the note "skip this part if not available" in the original description.

Now lsb_release is called to obtain the distro, instead of using utsname.version.

Screenshot from 2021-01-27 18-07-06

I wonder if it should be SysArchMeter? (up-case A)

Changed to SysArchMeter.

Edit: I also updated unsupported/Platform.c to include SysArchMeter.

@fasterit
Copy link
Member

None of this will change during the runtime of htop. So get the data once and be good with it?

@ahgamut
Copy link
Contributor Author

ahgamut commented Jan 27, 2021

Okay, that means I will need to use static variables to store the obtained info, resulting in something like:

static const int SysArchMeter_attributes[] = {HOSTNAME};
// use has_data to check if other variables have been loaded
static struct utsname uname_info;
static char distro_info[256];
static int uname_result;
static bool has_data = false;

Is there a way to do this without static variables?

SysArchMeter.c Outdated Show resolved Hide resolved
SysArchMeter.c Outdated Show resolved Hide resolved
SysArchMeter.c Outdated Show resolved Hide resolved
SysArchMeter.c Outdated Show resolved Hide resolved
openbsd/Platform.c Show resolved Hide resolved
SysArchMeter.c Outdated Show resolved Hide resolved
@BenBE
Copy link
Member

BenBE commented Jan 27, 2021

None of this will change during the runtime of htop. So get the data once and be good with it?

The LSB information /may/ change*. but even then it would suffice fetching it once every 5 minutes.

*When the package lsb-release is updated.

@ahgamut
Copy link
Contributor Author

ahgamut commented Jan 28, 2021

Thank you for your guidance @BenBE!
I'm summarizing the requested changes:

  • lsb_release -ircs instead of lsb_release -ds
  • sizeof instead of a hardcoded number
  • changed dashes to spaces when printing kernel and version
  • sort order and braces usage (conforming to style guide)
  • LSB information read just once at start

I'll push from my local system after confirming preferences on the last one.

Current screenshot:
Screenshot from 2021-01-28 11-37-53

@fasterit
Copy link
Member

* LSB information may change -- read just once, or check every 5 minutes?

Just once. If Ubuntu fixes a typo in their LSB package we can live with htop needing a restart.
If real LSB info changes (release version) people need a restart anyways which means htop will also (need to be) restarted.

SysArchMeter.c Outdated Show resolved Hide resolved
Copy link
Member

@fasterit fasterit left a comment

Choose a reason for hiding this comment

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

Looks, good. Thank you!

@BenBE
Copy link
Member

BenBE commented Jan 28, 2021

@fasterit, @ginggs, @natoscott: Maybe add Suggests: lsb-release to debian/control when packaging.

SysArchMeter.c Outdated Show resolved Hide resolved
SysArchMeter.c Outdated Show resolved Hide resolved
SysArchMeter.c Outdated Show resolved Hide resolved
SysArchMeter.c Outdated Show resolved Hide resolved
SysArchMeter.c Outdated Show resolved Hide resolved
@BenBE
Copy link
Member

BenBE commented Jan 31, 2021

Small note on the output of lsb_release: The order of the information returned is not guaranteed. Both lsb_release -irc and lsb_release -cri give the same output.

@BenBE BenBE added needs-rebase Pull request needs to be rebased and conflicts to be resolved new feature Completely new feature labels Jan 31, 2021
@ahgamut
Copy link
Contributor Author

ahgamut commented Jan 31, 2021

Small note on the output of lsb_release: The order of the information returned is not guaranteed. Both lsb_release -irc and lsb_release -cri give the same output.

lsb_release is now called thrice to avoid any ambiguity in the output order.

@fasterit
Copy link
Member

lsb_release is now called thrice to avoid any ambiguity in the output order.

Please no. Parse the prefixes as we do in htop in virtually anything the kernel exposes in /proc.
You can assume the prefixes to stay. At least as much as the command line switches.

@ahgamut ahgamut requested a review from BenBE January 31, 2021 13:09
Copy link
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.

I think we can do without the lsb_valid check and also keep the flexibility, that not every distribution release may have a codename assigned.

SysArchMeter.c Outdated Show resolved Hide resolved
At start, SysArchMeter calls the uname function to obtain the kernel
version and architecture. If available, the distro version is obtained
by calling lsb_release. The obtained values are stored in static
variables and used when updating the meter.
Copy link
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.

LGTM.

Thank you very much for your work on this feature.

@BenBE BenBE merged commit 51e79dd into htop-dev:master Jan 31, 2021
@ahgamut
Copy link
Contributor Author

ahgamut commented Feb 1, 2021

It's awesome that I got to contribute to htop.
Thanks @BenBE and @fasterit for your patience!

@BenBE
Copy link
Member

BenBE commented Feb 1, 2021

Always glad to see new people with their contributions and ideas. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Pull request needs to be rebased and conflicts to be resolved new feature Completely new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants