| blob | 0f6f8b47c01d565af8a1e6f562ec28f5dd13be39 |
1 Technical notes on my code submission of 26 Sep 99
2 Kern Sibbald
4 General:
5 - I've tried to follow the indenting standards existing for apcupsd,
6 but in reviewing my changes, I've noted several places where I
7 have followed the standards that I have used for many years, being
8 a bit lazy to correct them, I submit them as is, but if someone objects,
9 I'll be glad to fix them.
11 - I recommend that we discuss indenting standards. I find the current
12 standard of something like 6 of 7 spaces for each level of indentation
13 difficult to enter (it slows me down), difficult to read, and it
14 allows a lot less code per line. I propose a standard that indents
15 three spaces for each new indention level. This yields very readable
16 code and preserves space. Comments?
18 - I was displeased to discover two things about GNU software in making
19 this submission: 1. diff has no option to ignore tabs on input when
20 making comparisons. 2. make REQUIRES a tab in certain positions -- this
21 "problem" was fixed 10 years ago in all other versions of make with which
22 I have worked.
24 - In order to produce a minimum diff for this submission, I was forced to
25 remove all tabs from all .c and .h files. Unfortunately, due to the make
26 restrictions, I was not able to remove them from the Makefile.in files.
27 Consequently, for this submission, you will find that all lines in the
28 Makefile.in files that I changed are tabbed.
30 Changes to code: general
31 - This first set of changes attempts to implement two new error termination
32 routines error_abort() and error_exit(). Both are implemented as #defines
33 taking a variable number of arguments. I used defines, because they pass
34 the file and line number of the calling statement. Both ultimately use
35 the subroutine error_out(), which is a new subroutine located in apcupsd.c.
36 Note, for compilers such as the HP which apparently do not permit variable
37 arguments in #defines, I recommend that the HP coder, simply make some
38 HP defines as follows:
40 #define error_abort error_out
41 #define error_exit error_out
43 Then use ifdefs to change the definition of error_out to eliminate the
44 first three arguments keeping only the error varargs message passed
45 to it.
47 - I then cleaned up a number of files (mostly those called directly from
48 apcupsd.c) to use the new routines. There are still MANY other places
49 where these routines could be used. For example, apcserial.c takes great
50 care to return SUCCESS or FAILURE, but in doing so, the reason for any
51 failure is completely lost. Personally, I would prefer that apcserial.c
52 immediately error if it cannot continue. However, before making any
53 sweeping changes there, I would like to get the reactions of the other
54 developers to this code. The same applies to other files.
56 - Not implemented, but possible is to log the error messages rather than
57 fprintf them depending on whether or not the system log file is open.
58 I'll be glad to implement this idea depending on the reaction of the
59 developers.
61 - I modified all the routines called by terminate() to do nothing if they
62 were not initialized. This means that terminate() or error_abort,... can
63 be called from anywhere either before or after the program becomes a
64 daeon and starts the networking ... Again, some future work needs to be
65 done to use the log file rather than fprintf if apcupsd is running as
66 a daemon, but with my changes, it should be centralized in one or two
67 subroutines for the most part.
69 - I moved the initialization of the myUPS structure to very early in the
70 program. This permits proper checking of the state for early shutdowns.
72 - I added a sentinel to the beginning of the shared memory structure. This
73 permits distinguishing the old version and the new version when the
74 memory structure changes. I also added a version number for ensuring that
75 the programs such as apcaccess (and future .cgi programs) are using the
76 proper structure definition. I also added the size of the structure, and
77 the release number of apcupsd. This allows for full sanity checking.
79 - I updated the .man file. There seems to be a lot more work to do here.
80 Please give me your reactions.
82 - I cleaned up the apcupsd.config file a bit (removed obsolete configuration
83 statements) and added a bit more documentation.
85 - I added code to detect that the installation is being done on a RedHat 5.2
86 or 6.0 system (all the hooks were already there -- thanks). I also added
87 all the code necessary to do a correct make install on a RedHat system.
88 Please note that this is not tested yet on RedHat 5.2, so I am sure there
89 will need to be additional changes. In doing this, I moved the code (or
90 the logic) of /sbin/powersc into the apcupsd script -- this makes it
91 much more readable and eliminates the need for /sbin/powersc on RedHat
92 systems.
94 I recommend that the same be done in all other platforms and that we
95 eliminate entirely /etc/powersc. In the mean time, I would like to
96 see powersc moved to the distributions directory. That way it will not
97 be installed on RedHat systems.
99 - I like the new /usr/lib/apccontrol file. It is very nice. However, I
100 am a bit surprised that we eliminated the CONTROL configuration statement.
101 On RedHat systems, I would MUCH prefer to put this file in /etc/apcupsd
102 and to be able to configure its location in /etc/apcupsd.conf. Eventhough
103 /etc is getting a bit cluttered, most system administrators would prefer
104 that the really critical system stuff (such as apcupsd) be located in
105 etc. Can we resurrect the CONTROL code? Comments.
107 - Instead of erroring when obsolete configuration statements are found,
108 I made it print and error message and continue. This makes upgradding
109 a bit more pleasant.
111 - I removed the 10 second sleep in terminate() as in my opinion, it is
112 very undesirable. I have not yet had the time to test all the termination
113 cases, so there may be some subtile problems lurking in this change.
114 I'll test all the various shutdown cases in the next few weeks.
116 Final comments:
117 - I would like to get some feedback about what I did. Good/bad/indifferent?
119 - Now that I have a bit of experience in the code, I would like to
120 undertake a more "serious" project while at the same time continuing
121 some of the cleanup efforts that I have started.
123 Some projects that I consider "serious" are:
124 1. Implement PowerChute event logging.
125 2. Implement some way through a user program to query and
126 change the state of the UPS (e.g. Initiate UPS self test,
127 Initiate UPS runtime calibration, Simulate power failure,
128 test UPS alarm, ...)
129 3. Integrate the apcupscgi-1.0 code with apcupsd.
130 4. Implement .cgi programs that query and change the state
131 of the UPS.
132 5. Implement a general .cgi interface in place of the old
133 HTTP code.
134 6. Implement networking with APC PowerChute programs on other
135 machines (probably a .cgi program to keep the footprint of
136 apcupsd small).
138 Other projects already planned that I would like to continue
139 with little by little:
140 - Implementation of error_abort() throughout the code where
141 appropriate.
142 - Implementation of consistent logging as discussed in recent
143 emails.
144 - Adding additional documentation.
145 - Making a source and binary .rpm release for RedHat.
147 I would like to hear your comments on these and particularily what
148 the rest of you consider the priority of items 1 - 6.