On Tue, 2010-06-15 at 15:25 +0200, Karel Klic wrote:
Hi,
I have created a patch that improves our cron functionality:
- it fixes HH:MM cron option timing
- it removes a memory leak caused by unreleased event callback
-1 noise entry in Valgrind output
- some C++ was changed to C (only Settings are still in C++),
shorter and less complicated functions, some comments added
Looks good. A few comments below.
+ if (sscanf(time, "%d:%d", hours, minutes) != 2)
+ {
+ perror_msg("Failed to parse CRON time \"%s\"", time);
+ return -1;
+ }
(1) "%u:%u" format makes more sense - it will catch things like
"-1:-2"
(2) error_msg, not perror_msg - sscanf may not set errno, and
even if it does, we don't really win anything by showing
EINVAL or ERANGE to user in textual form - "badness" of the time
string will hopefully be obvious without it anyway:
Bad time specification "999999999999999999999:0"
is no worse than
Bad time specification "999999999999999999999:0": Value out of range
If you want to keep perror_msg, then set errno to 0 before sscanf:
perror_msg does not output strerror(srrno) if errno is 0.
(3) You can also check range:
if (sscanf(time, "%u:%u", hours, minutes) != 2 || (unsigned)hours > 23 ||
(unsigned)minutes > 59)
....
+ * Releasing memory to plugin_name and plugin_args
+ * should not ne needed after Settings rewrite to C.
^^^ typo
+ * Those pointers can point directly to the settings.
--
vda