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
Karel
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.
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.
crash-catcher@lists.fedorahosted.org