[wip] Inventory Linux and Windows crontasks #900
Conversation
There was a problem hiding this comment.
Hi @ddurieux
that's an interesting work.
Btw the code itself should be a lot improved.
And I finally saw during my review a problem in the way you're parsing crontab files under unix: files under /etc/cron.{daily,weekly,hourly,monthly} are not crontab file but generally scripts.
| my @files; | ||
| my @folders; | ||
| push @files, '/etc/crontab'; | ||
| push @folders, '/etc/cron.d'; | ||
| if (-d '/etc/cron.daily') { | ||
| push @folders, '/etc/cron.daily'; | ||
| } | ||
| if (-d '/etc/cron.hourly') { | ||
| push @folders, '/etc/cron.hourly'; | ||
| } | ||
| if (-d '/etc/cron.monthly') { | ||
| push @folders, '/etc/cron.monthly'; | ||
| } | ||
| if (-d '/etc/cron.weekly') { | ||
| push @folders, '/etc/cron.weekly'; | ||
| } |
There was a problem hiding this comment.
You should better initialize arrays here with something like.
| my @files; | |
| my @folders; | |
| push @files, '/etc/crontab'; | |
| push @folders, '/etc/cron.d'; | |
| if (-d '/etc/cron.daily') { | |
| push @folders, '/etc/cron.daily'; | |
| } | |
| if (-d '/etc/cron.hourly') { | |
| push @folders, '/etc/cron.hourly'; | |
| } | |
| if (-d '/etc/cron.monthly') { | |
| push @folders, '/etc/cron.monthly'; | |
| } | |
| if (-d '/etc/cron.weekly') { | |
| push @folders, '/etc/cron.weekly'; | |
| } | |
| my @files = qw(/etc/crontab); | |
| my @folders = grep { -d $_ } qw( | |
| /etc/cron.d | |
| /etc/cron.hourly | |
| /etc/cron.daily | |
| /etc/cron.weekly | |
| /etc/cron.monthly | |
| ); |
As in my suggestion, you should even test /etc/cron.d, this is just safe and will avoid an error if the folder finally doesn't exist.
There was a problem hiding this comment.
You should better initialize arrays here with something like.
As in my suggestion, you should even test/etc/cron.d, this is just safe and will avoid an error if the folder finally doesn't exist.
Nothing to add to the previous suggestion, except for the paths of the Users crontabs -- for RHEL and Debian based distribs.
| my @files; | |
| my @folders; | |
| push @files, '/etc/crontab'; | |
| push @folders, '/etc/cron.d'; | |
| if (-d '/etc/cron.daily') { | |
| push @folders, '/etc/cron.daily'; | |
| } | |
| if (-d '/etc/cron.hourly') { | |
| push @folders, '/etc/cron.hourly'; | |
| } | |
| if (-d '/etc/cron.monthly') { | |
| push @folders, '/etc/cron.monthly'; | |
| } | |
| if (-d '/etc/cron.weekly') { | |
| push @folders, '/etc/cron.weekly'; | |
| } | |
| my @files = qw(/etc/crontab); | |
| my @folders = grep { -d $_ } qw( | |
| /etc/cron.d | |
| /etc/cron.hourly | |
| /etc/cron.daily | |
| /etc/cron.weekly | |
| /etc/cron.monthly | |
| /var/spool/cron | |
| /var/spool/cron/crontabs | |
| ); |
| my $inventory = $params{inventory}; | ||
| my $logger = $params{logger}; | ||
|
|
||
| my %tasks; |
There was a problem hiding this comment.
Unused, remove it
| my %tasks; |
| my (%params) = ( | ||
| file => '/etc/crontab', | ||
| @_ | ||
| ); |
There was a problem hiding this comment.
seems not used to me
| my (%params) = ( | |
| file => '/etc/crontab', | |
| @_ | |
| ); |
| my $handle = getFileHandle(( | ||
| file => $filename, | ||
| @_ | ||
| )); | ||
| continue unless $handle; |
There was a problem hiding this comment.
Don't use continue directive but next even after an or like in:
| my $handle = getFileHandle(( | |
| file => $filename, | |
| @_ | |
| )); | |
| continue unless $handle; | |
| my $handle = getFileHandle( | |
| file => $filename, | |
| @_ | |
| ) | |
| or next; |
or just:
| my $handle = getFileHandle(( | |
| file => $filename, | |
| @_ | |
| )); | |
| continue unless $handle; | |
| my $handle = getFileHandle(( | |
| file => $filename, | |
| @_ | |
| )); | |
| next unless $handle; |
| if ($line =~ /^PATH/) { | ||
| $pathFound = 1; | ||
| next; | ||
| } | ||
| next if !$pathFound; |
There was a problem hiding this comment.
Is PATH really a constraint in crontab files ? When I read my man 5 crontab page, I see no ref for such a constraint.
I guess you may miss some tables then.
I see another problem here, you're parsing also files under /etc/cron.{daily,hourly,weekly,monthly}... These files are not in crontab format but are generally scripts.
So something is wrong with your "find" logic IMHO.
| } | ||
| if ($trigger->find('ScheduleByMonth/Months')) { | ||
| my @months; | ||
| my $byMonths = $trigger->findnodes('ScheduleByMonth/Months'); |
| if ($trigger->find('DaysOfWeek/Sunday')) { | ||
| push(@wdays, 0); | ||
| } | ||
| if ($trigger->find('DaysOfWeek/Monday')) { | ||
| push(@wdays, 1); | ||
| } | ||
| if ($trigger->find('DaysOfWeek/Tuesday')) { | ||
| push(@wdays, 2); | ||
| } | ||
| if ($trigger->find('DaysOfWeek/Wednesday')) { | ||
| push(@wdays, 3); | ||
| } | ||
| if ($trigger->find('DaysOfWeek/Thursday')) { | ||
| push(@wdays, 4); | ||
| } | ||
| if ($trigger->find('DaysOfWeek/Friday')) { | ||
| push(@wdays, 5); | ||
| } | ||
| if ($trigger->find('DaysOfWeek/Saturday')) { | ||
| push(@wdays, 6); | ||
| } |
There was a problem hiding this comment.
Too much if, the following is simpler and should work:
| if ($trigger->find('DaysOfWeek/Sunday')) { | |
| push(@wdays, 0); | |
| } | |
| if ($trigger->find('DaysOfWeek/Monday')) { | |
| push(@wdays, 1); | |
| } | |
| if ($trigger->find('DaysOfWeek/Tuesday')) { | |
| push(@wdays, 2); | |
| } | |
| if ($trigger->find('DaysOfWeek/Wednesday')) { | |
| push(@wdays, 3); | |
| } | |
| if ($trigger->find('DaysOfWeek/Thursday')) { | |
| push(@wdays, 4); | |
| } | |
| if ($trigger->find('DaysOfWeek/Friday')) { | |
| push(@wdays, 5); | |
| } | |
| if ($trigger->find('DaysOfWeek/Saturday')) { | |
| push(@wdays, 6); | |
| } | |
| my $dow = 0; | |
| foreach my $dow_name (qw(Sunday Monday Tuesday Wednesday Thursday Friday Saturday)) { | |
| push @wdays, $dow if $trigger->find('DaysOfWeek/'.$dow_name); | |
| $dow++; | |
| } |
| if ($executionMonth eq '*/1') { | ||
| $executionMonth = '*'; | ||
| } | ||
| if ($executionDay eq '*/1') { | ||
| $executionDay = '*'; | ||
| } | ||
| if ($executionHour eq '*/1') { | ||
| $executionHour = '*'; | ||
| } | ||
| if ($executionMinute eq '*/1') { | ||
| $executionMinute = '*'; | ||
| } | ||
| if ($executionWeekday eq '*/1') { | ||
| $executionWeekday = '*'; | ||
| } |
There was a problem hiding this comment.
You still assume vars are defined and there should be a cleaner way todo than using 5 times the same kind of if. I even don't see how $executionMonth & $executionWeekday could be set to '*/1'. Indeed day, hour and minute cases should be handled earlier, l.94, l.101 & l.108.
For example, l.94, try this:
$executionMinute = '*/'.$2;
could be replace by:
$executionMinute = $2 && $2 eq '1' ? '*' : '*/'.$2;
| my $status = 1; | ||
| if ($xp->find('/Task/Settings/Enabled')->string_value ne 'true') { | ||
| $status = 0; | ||
| } |
There was a problem hiding this comment.
Safer as nothing tell you the string_value call returns a defined value:
| my $status = 1; | |
| if ($xp->find('/Task/Settings/Enabled')->string_value ne 'true') { | |
| $status = 0; | |
| } | |
| my $enabled = $xp->find('/Task/Settings/Enabled')->string_value; | |
| my $status = $enabled && $enabled eq 'true' ? 1 : 0; |
The $status evaluation can also directly be integrated l.233.
| $executionMinute = ($2 + 0); | ||
| $executionHour = ($1 + 0); |
There was a problem hiding this comment.
If the purpose is to obtain clean integers:
| $executionMinute = ($2 + 0); | |
| $executionHour = ($1 + 0); | |
| $executionMinute = int($2); | |
| $executionHour = int($1); |
|
Hello, is this still a work in progress or is there any other way to retrieve crontabs ? Thanks in advance ^^ |
|
It will be integrated in June for the version 2.7 ;) |
|
Sorry if I'm a bit late here, but you're mixing directories with different kind of content here. Those directories contains configuration files for cron daemon, defining cron tasks:
Those directories contains scripts:
Those scripts are actually executed by a crontab entry generally defined in /etc/crontab or /etc/anacrontab, such as: If you want to enumerate cron tasks stricto senso, you should not include the content of the last directory group. If you want to enumerate everything executed by cron, which is different, you should make a difference between a task (ie, run-parts executed every hour) and the command executed by this task (ie, the list of scripts present in /etc/cron.hourly)/ |
No description provided.