Date: Wed, 5 Feb 1997 20:48:15 +0100 From: j@uriah.heep.sax.de (J Wunsch) To: ts@polynet.lviv.ua Cc: stevel@logues.rhn.orst.edu, FreeBSD-hackers@freebsd.org Subject: Re: UPS daemon Message-ID: <Mutt.19970205204815.j@uriah.heep.sax.de> In-Reply-To: <199702051719.TAA18357@NetSurfer.lp.lviv.ua>; from Slavik Terletsky on Feb 5, 1997 19:21:54 %2B0000 References: <199702051719.TAA18357@NetSurfer.lp.lviv.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
As Slavik Terletsky wrote:
> I managed to hack upsd. As most of ppl who has APC Smart UPS I use
> it in a dumb mode (I had no info on the cabling UPS to my box).
Ummm. You've reinvented the wheel. There's already a nice upsd
available, and it does indeed use the Smart UPS in smart mode (which
is *way* preferable over dumb mode, since those beasts are really
verbose in their reporting).
See ftp.ww.net.
Also, sorry to beat at you, but your program has many style violations
(not so terrible) and even some more serious problems.
> char *scr = "/etc/rc.shutdown";
Don't use this. Expect /etc/rc.shutdown to become a standard
component of FreeBSD anytime soon, to be called by init before
shutting down the system.
> /* check if script exists */
> if(!(fd = fopen(scr, "r"))) {
> fprintf(stderr, "fopen: %s: %s\n", scr, sys_errlist[errno]);
> exit(1);
> }
> fclose(fd);
`fd' translates to `file descriptor' for many people, which is
something very different from an stdio file handle (of type `FILE *').
Use access(2) or stat(2) to see if a file exists, abusing stdio is
overkill.
Better use the err(3) function family instead of hacking your own.
Code using sys_errlist[] all over the place is probably stylistically
bad. (If at all, use a centralized error handling function for better
portability.)
> /* become a daemon */
> switch(fork()) {
> case -1: /* error */
> fprintf(stderr, "fork: %s\n", sys_errlist[errno]);
> exit(1);
> case 0: /* child */
> break;
> default: /* parent */
> exit(0);
> }
Hmm, but you don't actually become a daemon. This would require
calling setsid(2).
> /* monitor device */
> while(1) {
...
> sleep(1);
> }
Uh-oh. This is indeed a system hog. You examine the line status
once every second, thus preventing that daemon from being swapped.
A much better way is to have it making the line its controlling tty,
and arrange for the external logic dropping DCD to get the attention
of the process (i.e., when power fails). This way, the daemon can
sleep all the time (and can be swapped), but will be signalled with a
SIGHUP to wakeup.
> /* save our filesystem */
> system("/bin/sync");
> system("/bin/sync");
> system("/bin/sync");
Hmpf. :-) Did you ever look into /bin/sync? All it does is calling
sync(2). Anyway, there's no need to call it at all. Leave this to
init. ``Thou shalt sync thrice.'' is a very old myth, but it's pure
religion, nothing else. ;-)
> /* wait and then run script */
> sleep(wait);
> system(scr);
Also, send the appropriate signal to init instead.
Again, sorry for hacking on your code, please take my ramblings as
constructive criticism, not as offense.
--
cheers, J"org
joerg_wunsch@uriah.heep.sax.de -- http://www.sax.de/~joerg/ -- NIC: JW11-RIPE
Never trust an operating system you don't have sources for. ;-)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Mutt.19970205204815.j>