All proposed changes were made, the updated patch is attached. I'll commit it to git tomorrow if no more comments appear.
Thanks for the review.
K
Dne 16.6.2010 15:37, Denys Vlasenko napsal(a):
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.