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.