Created attachment 384266 [details, diff] New improved ebuild Hi, here's a new ebuild for net-analyzer/vnstat. It has the following improvements: 1) New runscript - Uses OpenRC's built-in start/stop function - Support for OpenRC's built-in status function added 2) Systemd support added (not tested, I don't use systemd, sorry) 3) Default configuration improved - "Daemon{User,Group}" is now set in configuration: - systemd and OpenRC users stay in sync - Previously, when manually starting vnstatd, you could mess up permissions, when you forget to specify the right user - PidFile location fixed (vnstat-1.12 set PidFile to "/var/run/vnstat/vnstatd/vnstatd.pid") 4) We don't install the cronjob per default: - It is not the recommended way to update vnStat databases - The old cronjob was deactivated by comments, but "/bin/bash" was still executed every hour. Not much load but unnecessary for most setups. - People who still uses the cronjob instead of vnstatd can manually install the cronjob from "/usr/share/$PN/vnstat.cron"
Created attachment 384312 [details, diff] vnstat-1.12-r1.patch vnstatd.tmpfile fixed.
The systemd related changes look ok... but about all the rest I guess maintainers will need to give their opinion :)
Comment on attachment 384312 [details, diff] vnstat-1.12-r1.patch Why is this a patch seemingly coming from /dev/null? What would be interesting is to see the difference between files/vnstatd.confd and files/vnstatd-r1.confd. Formatting it like a patch only adds noise and would be helpful only for a developer who would just dump the files and commit them.
(In reply to Jeroen Roovers from comment #3) > Why is this a patch seemingly coming from /dev/null? What would be > interesting is to see the difference between files/vnstatd.confd and > files/vnstatd-r1.confd. Formatting it like a patch only adds noise and would > be helpful only for a developer who would just dump the files and commit > them. I created the patch against the portage tree so that anybody interested could just apply the patch and see/test the new revision. Yes, I could have just edited previous files but then the person who would add it to portage would have more work to do. My intention was to relieve a developer... If that was wrong/is not wanted please tell me how I should do it (better) next time. Thanks!
We've been through this before, I think. Attaching a) the actual file and optionally b) a diff against the older version would help developers the most, as the changes are obvious when someone runs diff on the old and new, and b) allows people to annotate and discuss the changes in bugzilla.
Created attachment 393014 [details, diff] Cleaned up diff for Thomas D. changes + selinux Here's a git-format-patch of the changes in Thomas D. patches, with added selinux support that was committed to the ebuild after it.
Comment on attachment 393014 [details, diff] Cleaned up diff for Thomas D. changes + selinux >diff --git a/net-analyzer/vnstat/files/vnstatd.confd b/net-analyzer/vnstat/files/vnstatd-r1.confd >similarity index 10% >copy from net-analyzer/vnstat/files/vnstatd.confd >copy to net-analyzer/vnstat/files/vnstatd-r1.confd >index 1790141..33edf35 100644 >--- a/net-analyzer/vnstat/files/vnstatd.confd >+++ b/net-analyzer/vnstat/files/vnstatd-r1.confd >@@ -1,7 +1,18 @@ > # /etc/conf.d/vnstatd: config file for /etc/init.d/vnstatd > >-# Adjust scheduling priority on vnstatd (default: 0) >-#VNSTATD_NICELEVEL="0" >+# Configuration file >+VNSTATD_CONFIGFILE="/etc/vnstat.conf" > >-# Pass extra options to vnstatd >-#VNSTATD_EXTRAOPTS="--config /etc/vnstat.conf" >+# PID file >+VNSTATD_PIDFILE="/run/vnstat/vnstatd.pid" >+ >+# Options to vnstatd >+# See vnstatd(8) for more details >+# Notes: >+# * Do not specify another PIDFILE but use the variable above to change the location >+# * Do not specify another CONFIGFILE but use the variable above to change the location >+VNSTATD_OPTS="" >+ >+# Wait x milliseconds after starting and check that daemon is still running. >+# See start-stop-daemon(8) for more details >+SSD_STARTWAIT=500 Why this? It looks uncommon thing to set. Is vnstatd special in some way? >diff --git a/net-analyzer/vnstat/files/vnstatd.systemd b/net-analyzer/vnstat/files/vnstatd.systemd >new file mode 100644 >index 0000000..dedc474 >--- /dev/null >+++ b/net-analyzer/vnstat/files/vnstatd.systemd >@@ -0,0 +1,11 @@ >+[Unit] >+Description=vnstatd is a flexible and robust way to update vnStat databases This is not an advert, please use something more explanatory. >+ >+[Service] >+After=network-online.target >+User=vnstat Are you sure vnstat doesn't require root permissions on start? It drops permissions itself anyway. >+ExecStart=/usr/bin/vnstatd --nodaemon There could be use for Type=forking since the daemon does some preparations before forking. Then, Type=forking could help figure out start failures early :). >+ExecReload=/bin/kill -HUP $MAINPID >+ >+[Install] >+WantedBy=multi-user.target >diff --git a/net-analyzer/vnstat/files/vnstatd.tmpfile b/net-analyzer/vnstat/files/vnstatd.tmpfile >new file mode 100644 >index 0000000..5a2b2cb >--- /dev/null >+++ b/net-analyzer/vnstat/files/vnstatd.tmpfile >@@ -0,0 +1 @@ >+d /run/vnstat 0775 vnstat vnstat >diff --git a/net-analyzer/vnstat/vnstat-1.12.ebuild b/net-analyzer/vnstat/vnstat-1.12-r1.ebuild >similarity index 53% >copy from net-analyzer/vnstat/vnstat-1.12.ebuild >copy to net-analyzer/vnstat/vnstat-1.12-r1.ebuild >index 82ebca6..b4cc346 100644 >--- a/net-analyzer/vnstat/vnstat-1.12.ebuild >+++ b/net-analyzer/vnstat/vnstat-1.12-r1.ebuild >@@ -1,9 +1,9 @@ > # Copyright 1999-2014 Gentoo Foundation > # Distributed under the terms of the GNU General Public License v2 >-# $Header: /var/cvsroot/gentoo-x86/net-analyzer/vnstat/vnstat-1.12.ebuild,v 1.2 2014/11/02 20:28:20 jer Exp $ >+# $Header: $ > > EAPI=5 >-inherit toolchain-funcs user >+inherit toolchain-funcs user systemd > > DESCRIPTION="Console-based network traffic monitor that keeps statistics of network usage" > HOMEPAGE="http://humdi.net/vnstat/" >@@ -20,6 +20,7 @@ DEPEND=" > RDEPEND=" > ${DEPEND} > selinux? ( sec-policy/selinux-vnstatd ) >+ !<sys-apps/openrc-0.13 Not convinced this is a good idea. > " > > pkg_setup() { >@@ -29,12 +30,15 @@ pkg_setup() { > > src_prepare() { > tc-export CC >+ >+ sed -i 's:^DaemonUser.*:DaemonUser "vnstat":' cfg/vnstat.conf || die "Failed to set DaemonUser!" >+ sed -i 's:^DaemonGroup.*:DaemonGroup "vnstat":' cfg/vnstat.conf || die "Failed to set DaemonGroup!" >+ sed -i 's:^MaxBWethnone.*:# &:' cfg/vnstat.conf || die "Failed to comment out example!" >+ sed -i 's:vnstat[.]log:vnstatd.log:' cfg/vnstat.conf || die "Failed to adjust LogFile name!" >+ sed -i 's:^PidFile.*:PidFile "/run/vnstat/vnstatd.pid":' cfg/vnstat.conf || die "Failed to adjust PidFile directive!" This became a bit monstrous. How about using a patch instead? > } > > src_compile() { >- sed -i 's:vnstat[.]log:vnstatd.log:' cfg/vnstat.conf || die >- sed -i 's:vnstat[.]pid:vnstatd/vnstatd.pid:' cfg/vnstat.conf || die >- > emake CFLAGS="${CFLAGS}" $(usex gd all '') > } > >@@ -42,15 +46,18 @@ src_install() { > use gd && dobin src/vnstati > dobin src/vnstat src/vnstatd > >- exeinto /etc/cron.hourly >- newexe "${FILESDIR}"/vnstat.cron vnstat >+ exeinto /usr/share/${PN} >+ newexe "${FILESDIR}"/vnstat-r1.cron vnstat.cron > > insinto /etc > doins cfg/vnstat.conf > fowners root:vnstat /etc/vnstat.conf > >- newconfd "${FILESDIR}"/vnstatd.confd vnstatd >- newinitd "${FILESDIR}"/vnstatd.initd vnstatd >+ newconfd "${FILESDIR}"/vnstatd-r1.confd vnstatd >+ newinitd "${FILESDIR}"/vnstatd-r1.initd vnstatd >+ >+ systemd_newunit "${FILESDIR}"/vnstatd.systemd vnstatd.service >+ systemd_newtmpfilesd "${FILESDIR}"/vnstatd.tmpfile vnstatd.conf > > keepdir /var/lib/vnstat > fowners vnstat:vnstat /var/lib/vnstat >@@ -75,11 +82,16 @@ pkg_postinst() { > elog "and set correct permissions after that, e.g." > elog " chown -R vnstat:vnstat /var/lib/vnstat" > elog >- elog "Note: if an interface transfers more than ~4GB in" >- elog "the time between cron runs, you may miss traffic" >+ elog "It is highly recommended to use the included vnstatd to update your" >+ elog "vnStat databases." > elog >- elog "To update the interfaces database automatically with cron, uncomment" >- elog "lines in /etc/cron.hourly/vnstat and set cron job to run it as" >- elog "frequently as required. Alternatively you can use vnstatd. Init script" >- elog "was installed into /etc/init.d/vnstatd for your convenience." >+ elog "If you want to use the old cron way to update your vnStat databases," >+ elog "you have to install the cronjob manually:" >+ elog >+ elog " cp /usr/share/${PN}/vnstat.cron /etc/cron.hourly/vnstat" >+ elog >+ elog "Note: if an interface transfers more than ~4GB in" >+ elog "the time between cron runs, you may miss traffic." >+ elog "That's why using vnstatd instead of the cronjob is" >+ elog "the recommended way to update your vnStat databases." > } >-- >2.2.1 >
(In reply to Michał Górny from comment #7) > >--- a/net-analyzer/vnstat/files/vnstatd.confd > >+++ b/net-analyzer/vnstat/files/vnstatd-r1.confd > > [...] > > Why this? It looks uncommon thing to set. Is vnstatd special in some way? I added a dedicated VNSTATD_CONFIGFILE="/etc/vnstat.conf" option instead of using command line option "--config" to allow us to use OpenRC's "required_files" check. I added a dedicated VNSTATD_PIDFILE option to allow the user to change PID file location. Also, we need a PID file value to use OpenRC's built-in status function. The dedicated VNSTATD_OPTS option replaces the removed VNSTATD_EXTRAOPTS option. The SSD_STARTWAIT option was added because we cannot 100% be sure if the daemon exited after s-s-d called it due to a config/startup problem, from `man start-stop-daemon`: -w, --wait milliseconds Wait milliseconds after starting and check that daemon is still running. Useful for daemons that check configuration after forking or stopping race conditions where the pidfile is written out after forking. But to be honest, I added SSD_STARTWAIT due to a bug I found in vnstat-1.11. That's the version I filled this bug report against. Meanwhile, upstream fixed this problem in vnstat-1.12... > >diff --git a/net-analyzer/vnstat/files/vnstatd.systemd b/net-analyzer/vnstat/files/vnstatd.systemd > >new file mode 100644 > >index 0000000..dedc474 > >--- /dev/null > >+++ b/net-analyzer/vnstat/files/vnstatd.systemd > >@@ -0,0 +1,11 @@ > >+[Unit] > >+Description=vnstatd is a flexible and robust way to update vnStat databases > > This is not an advert, please use something more explanatory. Upstream added systemd support with 1.12, from their service file [1]: Description=vnStat network traffic monitor > >+ > >+[Service] > >+After=network-online.target > >+User=vnstat > > Are you sure vnstat doesn't require root permissions on start? It drops > permissions itself anyway. > > >+ExecStart=/usr/bin/vnstatd --nodaemon > > There could be use for Type=forking since the daemon does some preparations > before forking. Then, Type=forking could help figure out start failures > early :). Please feel free to apply your changes. I don't use systemd, so I cannot judge or test... > > DESCRIPTION="Console-based network traffic monitor that keeps statistics of network usage" > > HOMEPAGE="http://humdi.net/vnstat/" > >@@ -20,6 +20,7 @@ DEPEND=" > > RDEPEND=" > > ${DEPEND} > > selinux? ( sec-policy/selinux-vnstatd ) > >+ !<sys-apps/openrc-0.13 > > Not convinced this is a good idea. Yes, this was a bad decision. When suggesting the change, OpenRC 0.13 was out and I ask William if OpenRC will become stable within 30 days... he said yes, So I also changed -#!/sbin/runscript +#!/sbin/openrc-run ...and that's why I added the dependency on openrc-0.13. But as said this was a bad decision. Please revert and also undo the shebang change. > > " > > > > pkg_setup() { > >@@ -29,12 +30,15 @@ pkg_setup() { > > > > src_prepare() { > > tc-export CC > >+ > >+ sed -i 's:^DaemonUser.*:DaemonUser "vnstat":' cfg/vnstat.conf || die "Failed to set DaemonUser!" > >+ sed -i 's:^DaemonGroup.*:DaemonGroup "vnstat":' cfg/vnstat.conf || die "Failed to set DaemonGroup!" > >+ sed -i 's:^MaxBWethnone.*:# &:' cfg/vnstat.conf || die "Failed to comment out example!" > >+ sed -i 's:vnstat[.]log:vnstatd.log:' cfg/vnstat.conf || die "Failed to adjust LogFile name!" > >+ sed -i 's:^PidFile.*:PidFile "/run/vnstat/vnstatd.pid":' cfg/vnstat.conf || die "Failed to adjust PidFile directive!" > > This became a bit monstrous. How about using a patch instead? No problem. We can patch or provide the whole config file... Should I update my patch?
Created attachment 394772 [details, diff] New diff based on changes from vnstat-1.13 and Michał's feedback Hi, I updated the diff against vnstat-1.13 ebuild in tree. For easier review I didn't bumped files in FILESDIR. Changes in summary (vs the current version in tree): 1) systemd supported added. 2) Like said in comment 0, the runscript now uses OpenRC's built-in start/stop/status function. 3) Like said in comment 0, default configuration improved. 4) We no longer install the default cron job. See comment 0 for more details.
Was this forgotten? It would be nice to have a systemd service file added to the tree.
Now in repository via https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=9bedfaa050827b35da830925cfd8a2e1469b3411
