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 support for MidnightBSD to a number of plugins #1342

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

Conversation

laffer1
Copy link
Contributor

@laffer1 laffer1 commented Jul 7, 2020

No description provided.

@coveralls
Copy link

coveralls commented Jul 7, 2020

Pull Request Test Coverage Report for Build 3007

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 52.42%

Totals Coverage Status
Change from base Build 3000: 0.0%
Covered Lines: 1083
Relevant Lines: 2066

💛 - Coveralls

@h01ger
Copy link
Contributor

h01ger commented Jul 8, 2020 via email

@laffer1
Copy link
Contributor Author

laffer1 commented Jul 8, 2020

It seemed to be the pattern with the others. I can modify the node.d.freebsd scripts instead to do an OS check and skip the legacy freebsd 4 and lower cases instead.

I will have to hack up build.pl though since it's doing this and midnightbsd is a supported platform in perl.

File::Find::find( { wanted => &_find_plugins_wanted },
'plugins/node.d', "plugins/node.d.$^O" );

@h01ger
Copy link
Contributor

h01ger commented Jul 8, 2020 via email

Copy link
Collaborator

@sumpfralle sumpfralle 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 for your contribution!

After going through half of the plugins (and commenting them) I noticed, that these look very similar to the set of files below plugins/node.d.freebsd.

Maybe you could describe, where are the relevant differences? Maybe these can be incorporated without creating a code copy (which would increase the maintenance burden)?

if [ -n "$cpus" ] ; then
echo "yes"
else
echo "no"
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is always helpful for users, if no is suffixed with a reason (e.g. no (empty output of 'sysctl dev.cpu')).

reqcpus
for cpu in $cpus ; do
echo -n "CPU$cpu.value "
sysctl -n dev.cpu.$cpu.temperature | tr -d C
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please quote the variable. shellcheck will appreciate this ...

;;
config)
config
;;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please reduce indentation.

echo yes
exit 0
else
echo no
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a reason for no.

exit 0
fi
else
echo no
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add something like (executable 'sysctl' not found).

netstat -i -b -n | sed -n -e '/^faith/d' -e '/^lo[0-9]/d' -e '/^pf(log|sync)/d' -e '/<Link#[0-9]*>/s/\** .*//p'
exit 0
else
exit 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a helpful error message?

echo 'graph_category network'
echo "graph_info This graph shows the amount of errors and collisions on the $INTERFACE network interface."
echo 'ierrors.label Input Errors'
echo 'ierrors.type COUNTER'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please reduce indentation.

exit 0
fi;

/usr/bin/netstat -i -b -n -I $INTERFACE | awk '
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please quote $INTERFACE.

echo yes
exit 0
else
echo "no (/usr/bin/netstat not found)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please mention /sbin/ifconfig.

}'

else
/usr/bin/netstat -i -b -n -I $INTERFACE | awk '
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please quote $INTERFACE.

@laffer1
Copy link
Contributor Author

laffer1 commented Jul 9, 2020

I pushed a newer commit that removes the entire node.d.midnightbsd directory and uses the existing freebsd scripts with modifications to check uname output as needed.

Copy link
Collaborator

@sumpfralle sumpfralle left a comment

Choose a reason for hiding this comment

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

Thank your for minimizing your set of changes! This looks fine to me now.

Please take a look at my minor nitpicks below and please squash your commits into one.

Thank you!

# Ignore Memory Disks, CD-ROM drives and Floppy
continue
try:
verboselog('Trying '+drive+'...')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add spaces around operators (+). PEP8 (and flake8) would appreciate this.

OSV=`$SYSCTL_BIN -n kern.osrelease | cut -f1 -d.`
if [ "$OSV" = "4" ]; then
if [ $(uname -s) = "MidnightBSD" ]; then
STATUNITS=`$SYSCTL_BIN -n kern.clockrate | cut -f13 -d' '`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think, you mixed up tabs and spaces here.

Additionally it would be great, if you could quote $(uname -s) in order to avoid and potential whitespace issues (and to please shellcheck).

Copy link
Collaborator

Choose a reason for hiding this comment

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

After taking another look at it: maybe you can just add the condition to the branch for "$OSV -ge "5" below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the order so that the $(uname -s) = "MidnightBSD" || "$OSV -ge "5" check is before "$OSV" = "4" instead. Otherwise, when MidnightBSD hits version 4, things will break.

@@ -589,6 +589,23 @@ def find_smart_drives():
continue
except Exception as exc:
verboselog('Failed to list FreeBSD disks: {}'.format(exc))
elif os.uname()[0] == "MidnightBSD":
Copy link
Collaborator

Choose a reason for hiding this comment

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

After taking another look at this: I think, you can avoid code duplication here by simply changing the above condition from elif os.uname()[0] == "NetBSD" to elif os.uname()[0] in {"NetBSD", "MidnightBSD"}:.

And maybe customize the final error message as verboselog('Failed to list {} disks: {}'.format(os.uname()[0], exc))

@@ -187,6 +187,10 @@ if ($^O eq 'linux') {
opendir(DEV, '/dev');
@drives = grep /^(ada?|da)[0-9]+$/, readdir DEV;
closedir(DEV);
} elsif ($^O eq 'midnightbsd') {
Copy link
Collaborator

@sumpfralle sumpfralle Jul 9, 2020

Choose a reason for hiding this comment

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

This is the same as above - maybe just use } elsif (($^O eq 'freebsd') || ($^O eq 'midnightbsd')) {.

Every line counts :)

@laffer1
Copy link
Contributor Author

laffer1 commented Jul 13, 2020

I've made the changes requested.

@daftaupe
Copy link

Hello, is that still expected to be merged someday ?
It would be a good basis to add DragonFlyBSD support :)

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

Successfully merging this pull request may close these issues.

6 participants