Hi netmon herd, I bring a gift. Changes include: 1. Added {conf,init}.d scripts. Updates via crontab remain an option, but cron file moved back to /etc/cron.d/ because... 2. vnstatd (via initscript) and vnstat (via crontab) run as non-root user/group "vnstat". 3. Fix a couple incorrect doc paths and dodoc the example cgi script. 4. Misc quoting, user notice grammar/spelling fixups (no more "you", droped), etc. 5. Should be a smooth upgrade for existing users of the package. In subsequent posts I'll attach the ebuild, a diff between 1.7-r1 and the new ebuild (called 1.7-r2), the {conf,init}.d scripts and new crontab file. Comments, questions, suggestions welcome. LMK if you want me to do the ci (if/once accepted). Thanks!
Created attachment 195259 [details] vnstat-1.7-r2.ebuild
Created attachment 195260 [details, diff] vnstat-1.7-r1-to-r2.diff
Created attachment 195262 [details] vnstatd.initd
Created attachment 195263 [details] vnstatd.confd
Created attachment 195264 [details] vnstat.crontab
Thank you very much for your contribution, Gordon. This is a thing I was going to do but never got to it :) > Comments, questions, suggestions welcome. Since you asked... ;) 1. || die after sed, newins, newconfd, newinitd, newdoc, dodoc, doman. 2. You don't need to quote inside [[ ... ]]. Thus - if [[ -d ${ROOT}/var/spool/vnstat ]] ; then + if [[ -d "${ROOT}"/var/spool/vnstat ]]; then dropped. 3. - # compatibility for 1.1 ebuild + # compat for 1.1 ebuild compatibility is English word while compat is abbreviation. I dropped this change. 4. Due to portage feature to preserve owner (bug #141619) I've changed all long code you had under + # compat for <1.7-r2 ebuilds to chown -R vnstat:vnstat "${ROOT}/var/lib/vnstat" chown vnstat:vnstat "${ROOT}/var/run/vnstatd" 5. Cron script... Well I don't think we want to revert fix, documented in ChangeLog: 21 Jan 2005; Aaron Walker <ka0ttic@gentoo.org> files/vnstat.cron, vnstat-1.4.ebuild: Install cron script in /etc/cron.hourly where it belongs, as /etc/cron.d 1) is only supported by vixie, and 2) should contain system crontab entries, not bash scripts. Fixes bug 60711. 6. It better to use $() instead of ``: http://bash-hackers.org/wiki/doku.php/mirroring/bashfaq/082 and it's better to use $(<file) instead of `cat file`. 7. If you don't mind I dropped VNSTATD_PIDFILE in conf.d config since I can't think of any reason to change PID location. I've commited updated ebuild to the tree. If you have any further updates, don't hesitate to report them too. Thank you again!
Hi Peter, Looks good and thank you for all the tips! I will add them to my repertoire. I do have a couple issues though, detailed below. I admit to not looking at the changelog at all, but I did look at the cron guide to see which crons we offered. That totally sucks about fcron. I am not a user of it, but I was under the impression from the fcron FAQ that users were supposed to use that generation script which includes generating from /etc/cron.d. The problem is that vnstat/vnstatd create a backup interface file before updating the interface db. So if a user runs it via cron first and then decides to use the daemon, the interface db will get no updates until they fix the permissions on the /var/lib/vnstat/.interfacebackupfile. I was hoping to keep them from tripping over that. Secondly, the lowest period of time offered there is 1 hour (really 1hr, 5m). This breaks vnstat in my testing and it'll mark 0 traffic for an hour sometimes (about 3-4 times/day) and attribute the traffic to the next hour. The ebuild signals the user to run the job as frequently as required, but how are they supposed to do that without getting rid of the meaning hourly? How can we fix both these issues with the current vnstat crontab setup? Lastly, I really dislike 'you' or 'your' in ebuilds. It's a crutch/easy way out and its not really correct to personalize a message as it assumes the package installer is also the user. The warning about ~4GB in traffic should be moved to a place *after* the user is told there is even a cron script available IMO.
Oh, one more thing (and this was a mistake in my final ebuild too, not sure how vnstat:vnstat snuck in my final -exec) was that I wanted to avoid adjusting group ownership on vnstat db files in pkg_postinst() incase the user had set it to something else (say for a cgi script that runs as different user/group) and wants o-r for some reason. The db files really only need vnstat user ownership in order for vnstat/vnstatd to be able to update the db files. By adjusting group ownership, the ebuild will break user-adjusted group ownership of the files every time the package is reinstalled/updated.
> I admit to not looking at the changelog at all, but I did look at the cron > guide to see which crons we offered. That totally sucks about fcron. I am not > a user of it, but I was under the impression from the fcron FAQ that users were > supposed to use that generation script which includes generating from > /etc/cron.d. The problem is that vnstat/vnstatd create a backup interface file > before updating the interface db. So if a user runs it via cron first and then > decides to use the daemon, the interface db will get no updates until they fix > the permissions on the /var/lib/vnstat/.interfacebackupfile. I was hoping to > keep them from tripping over that. > > Secondly, the lowest period of time offered there is 1 hour (really 1hr, 5m). > This breaks vnstat in my testing and it'll mark 0 traffic for an hour sometimes > (about 3-4 times/day) and attribute the traffic to the next hour. The ebuild > signals the user to run the job as frequently as required, but how are they > supposed to do that without getting rid of the meaning hourly? > > How can we fix both these issues with the current vnstat crontab setup? Considering fcron is not nearly as popular a choice as the others, I don't see any reason to screw things up for the vast majority of users to accommodate fcron users. If these issues cannot be overcome I'd suggest the new cron script in /etc/cron.d and one of the following: 1. Patch fcron in gentoo to use /etc/cron.d (at least one other ebuild in the tree uses this dir too iirc). Really, the flexibility of /etc/cron.d is valuable in gentoo IMO and should be supported by all cron daemons in the tree. 2. ewarn the user that if using fcron they need to one of the following: a) replace their cron daemon b) use vnstatd c) use the generate script from the fcron FAQ. Otherwise I'd suggest get rid of the cron script all together, because it doesn't even work properly the way it is now. I'd prefer not to go this route, but I still think its better than the current situation.
(In reply to comment #7) > How can we fix both these issues with the current vnstat crontab setup? Both issues are documented in elog. User have to bother with permissions. But... may be it's better to move permissions assignment into init script. If you agree I'll do that. > Lastly, I really dislike 'you' or 'your' in ebuilds. Please, provide patch :) English is not my native language, but still I don't like how the following sounds: + elog "One can set the database directory using the \"DatabaseDir\"" + elog "directive in the configuration file (/etc/vnstat.conf)." (In reply to comment #8) > adjusting group ownership on vnstat db files in pkg_postinst() incase the > user had set it to something else (say for a cgi script that runs as > different user/group) and wants o-r for some reason. What if we change to chmod -R vnstat /var/lib/vnstat? In such a case group permissions are open for users. > Considering fcron is not nearly as popular a choice as the others, That's wrong assumption. It's best cron ever ;) And we already had bug I've mentioned right? > Otherwise I'd suggest get rid of the cron script all together, because it > doesn't even work properly Works for me. Also read elog: "and set cron job to run it as frequently as required". W e put in in hourly since I don't have better location. Probably we need support /etc/cron.d/ somehow in fcron (and probably in other cron daemons we have in the tree. I haven't checked but probable dcron does not support this directory too) but that's discussion should be opened in new bug.
(In reply to comment #10) > (In reply to comment #7) > > How can we fix both these issues with the current vnstat crontab setup? > > Both issues are documented in elog. User have to bother with permissions. > But... may be it's better to move permissions assignment into init script. If > you agree I'll do that. The permissions issue is not documented by elog. I think maybe I wasn't quite clear about what I meant. Let's say you install vnstat and run vnstat -u -i eth0 ; chown -R vnstat /var/lib/vnstat. You'd then have this: vnstat:root /var/lib/vnstat/eth0 With the current cron setup, vnstat will run with root permissions. So on the next vnstat -u you will have this: root:root /var/lib/vnstat/.eth0 vnstat:root /var/lib/vnstat/eth0 Where .eth0 is the backup vnstat creates before updating the 'eth0' database. So then the user decides cron updates suck for vnstat and wants to use vnstat via init script. vnstatd will again to backup eth0 to .eth0 and fail. It will then not update eth0 since it couldn't make a backup of it first. The user is left to figure out why its failing. Running the cronjob as user vnstat (which can only be done via /etc/cron.d) fixes this. The second issue is that the script is dumped in /etc/cron.hourly (which is broken for vnstat). So if the user wants to use cron to update vnstat dbs, they are left to make everything in /etc/cron.hourly run on a less-than-hourly basis (via /etc/crontab edit) or stick with the brokenness. Both are horrible solutions. Yes, the permission problem can be solved by modifying the init script, but that's also a lameish workaround IMO. The updates should just run as user vnstat no matter what route (vnstatd/cron) is taken. > > > Lastly, I really dislike 'you' or 'your' in ebuilds. > > Please, provide patch :) English is not my native language, but still I don't > like how the following sounds: > > + elog "One can set the database directory using the \"DatabaseDir\"" > + elog "directive in the configuration file (/etc/vnstat.conf)." > I will gladly provide a patch to the ebuild to clean/re-arrange elog messages (slightly). As a native english speaker (which you speak quite well too I might add) I cannot see the issue with the above sentence. May I ask what irks you about it and I'll see if I can't work around it? > (In reply to comment #8) > > adjusting group ownership on vnstat db files in pkg_postinst() incase the > > user had set it to something else (say for a cgi script that runs as > > different user/group) and wants o-r for some reason. > > What if we change to chmod -R vnstat /var/lib/vnstat? In such a case group > permissions are open for users. Yes, that's what I had in mind. I had that originally in the ebuild, but I think I might've copy/pasted into the ebuild from some testing in another console. My bad, sorry about that. > > > Considering fcron is not nearly as popular a choice as the others, > > That's wrong assumption. It's best cron ever ;) And we already had bug I've > mentioned right? Even if I agreed with you there (I don't, because it doesn't support /etc/cron.d by default :p) I still don't think its one of the most *popular*/*most used* choices. I'd wager vixie-cron, dcron and anacron make up the vast majority of cron installations. And yes, there's that bug, but that doesn't stop one from changing things in the future to the betterment of 95% of users and ewarning the other 5% (fcron users :p) > > > Otherwise I'd suggest get rid of the cron script all together, because it > > doesn't even work properly > > Works for me. Also read elog: "and set cron job to run it as frequently as > required". W e put in in hourly since I don't have better location. > *Kind of* works until you switch from cron to vnstatd. ;) Although all that missed traffic is really annoying, kind of breaks the point of vnstat. Fixing it breaks the meaning of /etc/cron.hourly. > > Probably we need support /etc/cron.d/ somehow in fcron (and probably in other > cron daemons we have in the tree. I haven't checked but probable dcron does not > support this directory too) but that's discussion should be opened in new bug. > Agreed (though its beyond my ability to do *securely*) and also agree its a discussion for another bug. :) Looking forward to helping anyway I can. Thanks Peter!
Is there any reason why the cron entry should be kept included in the ebuild? I don't see any benefits in having an update option (especially as the first suggestion) included that's known to be broken for many users. The daemon is there to fix these kind of issues since it's properly configured by default and doesn't have a dependency to any cron features. That file ownership issue could be easily fixed be adding a chown line to the start (and possibly also reload) section of the init.d script. That way the user wouldn't need to remember any extra commands when later adding new interfaces to vnStat and there would also be less elog prints.
(In reply to comment #12) > Is there any reason why the cron entry should be kept included in the ebuild? I > don't see any benefits in having an update option (especially as the first > suggestion) included that's known to be broken for many users. The daemon is > there to fix these kind of issues since it's properly configured by default and > doesn't have a dependency to any cron features. > This is pretty much what I stated @ the end of comment #9; Not providing a cron script is better than providing one that is breaks things/doesn't work properly (time intervals). However, if the plan is not to include a cron script at all, then why not include one that *does* work proper/doesn't break things? Even if it only works for a majority of the userbase and we have to ewarn for the remaining, it is better to provide options to the user where possible no? I prefer either option though over the current offering.
(In reply to comment #11) > Running the cronjob as user vnstat (which can only be done via /etc/cron.d) Ah, now I see. It _is_ possible to run cron job as vnstat user. For me it's evident that cronjob should be ran from vnstat user.... but I'll update elog message to reflect this. > Fixing it breaks the meaning of /etc/cron.hourly. Well, I agree with you. I don't think that /etc/cron.hourly is a best way to put cronscript. What do you think if we: 1. We put current cron script into /usr/bin/vnstat-cron 2. Put vnstat.crontab but instead of script, we'll put call for /usr/bin/vnstat-cron there. Does this solve the issue? (In reply to comment #12) > Is there any reason why the cron entry should be kept included in the ebuild? It works for me so I don't see any reason running additional daemon where cronjob suffice. If you don't want cron job just use fcrond.
Hi Peter, Sorry for the delay in getting back. Responses inline. (In reply to comment #14) > (In reply to comment #11) > > Running the cronjob as user vnstat (which can only be done via /etc/cron.d) > > Ah, now I see. It _is_ possible to run cron job as vnstat user. For me it's > evident that cronjob should be ran from vnstat user.... but I'll update elog > message to reflect this. > Possible to run as vnstat user from /etc/cron.{hourly,daily,weekly,monthly}? > > Fixing it breaks the meaning of /etc/cron.hourly. > > Well, I agree with you. I don't think that /etc/cron.hourly is a best way to > put cronscript. What do you think if we: > > 1. We put current cron script into /usr/bin/vnstat-cron > 2. Put vnstat.crontab but instead of script, we'll put call for > /usr/bin/vnstat-cron there. > > Does this solve the issue? If you want to call an external script (run as vnstat user) instead that's ok with me. But where would the crontab reside (as this is the real issue)? > > (In reply to comment #12) > > Is there any reason why the cron entry should be kept included in the ebuild? > > It works for me so I don't see any reason running additional daemon where > cronjob suffice. If you don't want cron job just use fcrond. > I totally agree with the 'lets give users options and let them choose' sentiment. As long as those options don't create confusing situations/bugs. I'd rather see it gone than the current situation. But offering it in a better way that may not work for fcron users is still better than not offering it at all. However, afaict the daemon is even more lightweight than using vnstat -u. Go figure. ;)
(In reply to comment #15) > However, afaict the daemon is even more lightweight than using vnstat -u. Go > figure. ;) I don't have any real figures to confirm which method is actually lighter. The daemon basically runs the same functions as vnstat -u in a loop between sleeps so as a result the daemon is sleeping most of the time. The difference is that it doesn't write the result to disk after each update but keeps the data cached instead for a configurable period of time and also tracks the availability of the interface. At least from my point of view that's what makes the daemon better especially for laptop users. Running vnstat -u causes a spinup of the hd for each update compared to the daemon that only needs one write once every hour. Essentially with the daemon you sacrifice a small amount of memory (less than the size of the cron daemon) and cpu time but gain reduced disk i/o and more configurability.
In the mean time I bumped to 1.9 re bug #297565, basically copying vnstat-1.7-r2.ebuild.
Fixed long ago I think.
