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

Add NetBSD support in htop(1) #603

Merged
merged 20 commits into from
Jun 26, 2021
Merged

Conversation

fraggerfox
Copy link
Contributor

This implementation makes NetBSD use htop(1) without the need of mount_procfs(8).

The implementation has been copied over and modified from the OpenBSD implementation in htop(1).

Make NetBSD no longer masquerade as Linux.

netbsd/Platform.c Outdated Show resolved Hide resolved
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.

Thank you very much for your implementation to support NetBSD natively for htop.

As there's one bigger change upcoming that affects all current platforms (and thus also all upcoming ones, I took the liberty to remark on the locations where this potentially requires changes to your PR.

netbsd/Platform.c Outdated Show resolved Hide resolved
netbsd/NetBSDProcess.c Show resolved Hide resolved
netbsd/NetBSDProcess.c Outdated Show resolved Hide resolved
netbsd/NetBSDProcess.h Outdated Show resolved Hide resolved
netbsd/NetBSDProcess.h Outdated Show resolved Hide resolved
netbsd/NetBSDProcessList.c Outdated Show resolved Hide resolved
netbsd/Platform.c Outdated Show resolved Hide resolved
netbsd/Platform.c Outdated Show resolved Hide resolved
netbsd/Platform.c Outdated Show resolved Hide resolved
netbsd/NetBSDProcessList.c Show resolved Hide resolved
@BenBE BenBE added BSD 🐡 Issues related to *BSD needs-discussion 🤔 Changes need to be discussed and require consent new feature Completely new feature labels Apr 17, 2021
@fraggerfox
Copy link
Contributor Author

@BenBE Thank you for going through the code and giving a detailed feedback, let me go through each of them and try to resolve it.

@BenBE
Copy link
Member

BenBE commented Apr 17, 2021

@fraggerfox Another point that came up in discussions in the team was the issue of long-term support of the platform. Would you be willing to help mid-/long-term to keep the NetBSD port up-to-date and aid in debugging issues specific to this platform? Or do you know someone who would be willing to take up on this task?

@fraggerfox
Copy link
Contributor Author

@fraggerfox Another point that came up in discussions in the team was the issue of long-term support of the platform. Would you be willing to help mid-/long-term to keep the NetBSD port up-to-date and aid in debugging issues specific to this platform? Or do you know someone who would be willing to take up on this task?

Yes, I do not mind putting in my time mid-/long-term to support the NetBSD port. Hopefully, I should have more time to commit to maintenance in the coming months. In case of issues / interruptions I can notify the team ahead of time.

@alarixnia
Copy link
Contributor

In general I think there's lots of interest in this, and I'm also able to help. We're also in #netbsd-code on freenode and can answer questions about APIs and offer testing.

@fraggerfox fraggerfox requested a review from BenBE April 24, 2021 04:49
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.

Changes LGTM.

netbsd/Platform.c Outdated Show resolved Hide resolved
netbsd/NetBSDProcessList.c Outdated Show resolved Hide resolved
Makefile.am Outdated Show resolved Hide resolved
Makefile.am Outdated Show resolved Hide resolved
netbsd/Platform.h Outdated Show resolved Hide resolved
@fraggerfox fraggerfox requested a review from BenBE April 25, 2021 17:29
XUtils.h Outdated Show resolved Hide resolved
netbsd/NetBSDProcessList.c Outdated Show resolved Hide resolved
netbsd/NetBSDProcessList.c Outdated Show resolved Hide resolved
@BenBE BenBE force-pushed the netbsd-no-procfs branch 2 times, most recently from 39136a8 to 7509af3 Compare April 27, 2021 17:16
@fraggerfox fraggerfox requested a review from cgzones April 27, 2021 19:24
netbsd/NetBSDProcess.h Outdated Show resolved Hide resolved
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.

Some details still need refinement.

Also may benefit to include ELAPSED column from #627.

Furthermore PROC_EXE (via KERN_PROC_PATHNAME) and CWD (via KERN_PROC_CWD) might be nice to have.

netbsd/Platform.c Outdated Show resolved Hide resolved
netbsd/NetBSDProcess.c Show resolved Hide resolved
netbsd/NetBSDProcessList.c Outdated Show resolved Hide resolved
netbsd/NetBSDProcessList.h Outdated Show resolved Hide resolved
netbsd/NetBSDProcessList.c Show resolved Hide resolved
netbsd/Platform.c Show resolved Hide resolved
netbsd/Platform.h Show resolved Hide resolved
netbsd/Platform.h Show resolved Hide resolved
netbsd/README.md Outdated Show resolved Hide resolved
@BenBE BenBE removed the needs-discussion 🤔 Changes need to be discussed and require consent label Jun 1, 2021
netbsd/NetBSDProcessList.c Outdated Show resolved Hide resolved
@fraggerfox fraggerfox requested a review from BenBE June 13, 2021 00:16
@BenBE
Copy link
Member

BenBE commented Jun 13, 2021

@fraggerfox Did some commit mashing ;-) No functional changes.

@fraggerfox
Copy link
Contributor Author

@fraggerfox Did some commit mashing ;-) No functional changes.

Looks good.

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.

Source-wise it LGTM. Haven't tested to run this though.

@alarixnia
Copy link
Contributor

alarixnia commented Jun 13, 2021

fraggerfox asked me to test his code, but unfortunately htop fails to configure with NetBSD libcurses.

checking for ncursesw/curses.h... no
checking for ncurses/ncurses.h... no
checking for ncurses/curses.h... no
checking for ncurses.h... no
configure: error: can not find required ncurses header file

this should be unnecessary, as NetBSD libcurses should be good enough.

so, I re-ran configure with export CFLAGS="-I/usr/pkg/include" and export LDFLAGS="-L/usr/pkg/lib -Wl,-R/usr/pkg/lib" so it would find a copy of ncurses I installed.

after this it appears to work fine, although the output is still less useful than top(1) - it groups the file cache together with active memory used by applications, and it does not show kernel threads. it is definitely not a regression from previous htop on netbsd, though

@BenBE
Copy link
Member

BenBE commented Jun 13, 2021

@fraggerfox asked me to test his code, but unfortunately htop fails to configure with NetBSD libcurses.

checking for ncursesw/curses.h... no
checking for ncurses/ncurses.h... no
checking for ncurses/curses.h... no
checking for ncurses.h... no
configure: error: can not find required ncurses header file

this should be unnecessary, as NetBSD libcurses should be good enough.

so, I re-ran configure with export CFLAGS="-I/usr/pkg/include" and export LDFLAGS="-L/usr/pkg/lib -Wl,-R/usr/pkg/lib" so it would find a copy of ncurses I installed.

Looking through configure.ac we only ever look for the ncurses headers, but not for any headers only called curses.h. Can you have a shot at updating configure.ac to also look for curses.h as a last resort? Also have a look at ProvideCurses.h to see if this needs any updates. That's the only place in htop that directly includes any of the ncurses/curses headers.

after this it appears to work fine, although the output is still less useful than top(1) - it groups the file cache together with active memory used by applications,

This looks like when reading the memory stats some details are still missing. Actually there's a second open point (regarding shared memory) … @Niacat Could you have a look at that?

and it does not show kernel threads. It is definitely not a regression from previous htop on netbsd, though

Can you see if the check for kernel threads in netbsd/NetBSDProcessList.c (here) is done properly?

@fraggerfox
Copy link
Contributor Author

fraggerfox commented Jun 13, 2021

fraggerfox asked me to test his code, but unfortunately htop fails to configure with NetBSD libcurses.

checking for ncursesw/curses.h... no
checking for ncurses/ncurses.h... no
checking for ncurses/curses.h... no
checking for ncurses.h... no
configure: error: can not find required ncurses header file

this should be unnecessary, as NetBSD libcurses should be good enough.

so, I re-ran configure with export CFLAGS="-I/usr/pkg/include" and export LDFLAGS="-L/usr/pkg/lib -Wl,-R/usr/pkg/lib" so it would find a copy of ncurses I installed.

@Niacat When running the build via the pkgsrc infrastructure these flags are passed in, I guess this is because of USE_NCURSES= yes . When I was building it outside pkgsrc I passed in the flags manually as you have described above.

after this it appears to work fine, although the output is still less useful than top(1) - it groups the file cache together with active memory used by applications, and it does not show kernel threads. it is definitely not a regression from previous htop on netbsd, though

Regarding the memory usage I mostly referred to top(1)'s code and uvm((9)' sysctl interface to get the necessary values.

For the kernel threads, yes I agree, it pretty much does what the current htop(1) does in NetBSD, is there a better way to process this information?

@alarixnia
Copy link
Contributor

We should probably get this merged and improve things further later.

In particular getting it to build with X/Open Curses seems difficult - it is peeking into ncurses-specific structs.

@BenBE BenBE added the NetBSD 🎏 NetBSD related issues label Jun 17, 2021
@BenBE
Copy link
Member

BenBE commented Jun 17, 2021

Could you open issues for the remaining items on your list? What I see is:

@fasterit fasterit added this to the 3.1.0 milestone Jun 18, 2021
@fraggerfox
Copy link
Contributor Author

I opened a couple more tickets for the issues raised.

@BenBE BenBE merged commit 3bed682 into htop-dev:master Jun 26, 2021
@fraggerfox fraggerfox deleted the netbsd-no-procfs branch August 4, 2021 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BSD 🐡 Issues related to *BSD NetBSD 🎏 NetBSD related issues new feature Completely new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants