Minor inline notes.
As for compute_next_events: I like its structure and the looping makes
it robust (at least it's harder to forget to update time-piece
variables if we add dates etc. when sticking to this style), but it
still makes me want to shorten a bit.
Wouldn't really make it more readable though.
Inlining check_hour() and removing its loop would not make that part
much shorter and one would have to make sure to adapt $hour and $min
(and if we add dates, $year, $mon and $mday as well probably).
Inlining check_minute() could work and save 1 iteration but I'm not sure
it's worth the change.
As for its termination... as long as hspec/mspec can't be empty arrays
(the corresponding parser cases should all error AFAICT) it should work.
On Wed, May 17, 2017 at 07:29:35AM +0200, Dietmar Maurer wrote:
> Not complete, but useful.
>
> Signed-off-by: Dietmar Maurer <diet...@proxmox.com>
> ---
> src/Makefile | 1 +
> src/PVE/CalendarEvent.pm | 217
> +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 218 insertions(+)
> create mode 100644 src/PVE/CalendarEvent.pm
>
> diff --git a/src/Makefile b/src/Makefile
> index a45effb..05344f5 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -7,6 +7,7 @@ MAN1DIR=${MANDIR}/man1/
> PERLDIR=${PREFIX}/share/perl5
>
> LIB_SOURCES= \
> + CalendarEvent.pm \
> OTP.pm \
> Ticket.pm \
> RESTEnvironment.pm \
> diff --git a/src/PVE/CalendarEvent.pm b/src/PVE/CalendarEvent.pm
> new file mode 100644
> index 0000000..cda3098
> --- /dev/null
> +++ b/src/PVE/CalendarEvent.pm
> @@ -0,0 +1,217 @@
> +package PVE::CalendarEvent;
> +
> +use strict;
> +use warnings;
> +use Data::Dumper;
> +use Time::Local;
> +
> +# Note: This class implements a parser/utils for systemd like calender exents
> +# Date specification is currently not implemented
> +
> +my $dow_names = {
> + sun => 0,
> + mon => 1,
> + tue => 2,
> + wed => 3,
> + thu => 4,
> + fri => 5,
> + sat => 6,
> +};
> +
> +# The parser.
> +# returns a $calspec hash which can be passed to compute_next_event()
> +sub parse_calendar_event {
> + my ($event) = @_;
> +
> + my $parse_single_timespec = sub {
> + my ($p, $max, $matchall_ref, $res_hash) = @_;
> +
> + if ($p =~ m/^((?:\*|[0-9]+))(?:\/([1-9][0-9]*))?$/) {
> + my ($start, $repetition) = ($1, $2);
> + if (defined($repetition)) {
> + $repetition = int($repetition);
> + $start = $start eq '*' ? 0 : int($start);
> + die "value '$start' out of range\n" if $start >= $max;
> + die "repetition '$repetition' out of range\n" if $repetition >=
> $max;
> + while ($start < $max) {
> + $res_hash->{$start} = 1;
> + $start += $repetition;
> + }
> + } else {
> + if ($start eq '*') {
> + $$matchall_ref = 1;
> + } else {
> + $start = int($start);
> + $res_hash->{$start} = 1;
> + }
> + }
> + } elsif ($p =~ m/^([0-9]+)\.\.([1-9][0-9]*)$/) {
> + my ($start, $end) = (int($1), int($2));
> + die "range start '$start' out of range\n" if $start >= $max;
> + die "range end '$end' out of range\n" if $end >= $max || $end <
> $start;
> + for (my $i = $start; $i <= $end; $i++) {
> + $res_hash->{$i} = 1;
> + }
> + } else {
> + die "unable to parse calendar event '$p'\n";
> + }
> + };
> +
> + my $h = undef;
> + my $m = undef;
> +
> + my $matchall_minutes = 0;
> + my $matchall_hours = 0;
> + my $minutes_hash = {};
> + my $hours_hash = {};
> +
> + my $dowsel = join('|', keys %$dow_names);
> +
> + my $dow_hash;
> +
> + my $parse_dowspec = sub {
> + my ($p) = @_;
> +
> + if ($p =~ m/^($dowsel)$/i) {
> + $dow_hash->{$dow_names->{lc($1)}} = 1;
> + } elsif ($p =~ m/^($dowsel)\.\.($dowsel)$/i) {
> + my $start = $dow_names->{lc($1)};
> + my $end = $dow_names->{lc($2)} || 7;
> + if ($end < $start) {
> + my $t = $start; $start = $end; $end = $t;
This causes Fri..Mon to become Mon..Fri, while I'd expect it to be
Fri,Sat,Sun,Mon.
In systemd, however, this is an error, so let's do the same.
> + }
> + for (my $i = $start; $i <= $end; $i++) {
> + $dow_hash->{($i % 7)} = 1;
> + }
> + } else {
> + die "unable to parse weekday specification '$p'\n";
> + }
> + };
> +
> + my @parts = split(/\s+/, $event);
> +
> + if ($parts[0] =~ m/$dowsel/i) {
> + my $dow_spec = shift @parts;
> + foreach my $p (split(',', $dow_spec)) {
> + $parse_dowspec->($p);
> + }
> + } else {
> + $dow_hash = { 0 => 1, 1 => 1, 2 => 1, 3 => 1, 4 => 1, 5=> 1, 6 => 1 };
> + }
> +
> + if (scalar(@parts) && $parts[0] =~ m/\-/) {
> + my $date_spec = shift @parts;
> + die "date specification not implemented";
> + }
> +
> + my $time_spec = shift(@parts) // "00:00";
> + my $chars = "[0-9\*\/\.,]";
You're using double quotes, which means backslashes get interpreted
here. Better use single quotes. You don't need to escape * or . inside
character classes, and when coming from a variable, / also doesn't need
escaping. (Consider the possibility that $chars is used in a `m,$chars,`
or `m<$chars>` statement rather than with slashes as in /$chars/, then
having to escape commas or < and > would be annoying ;-) )
> + if ($time_spec =~ m/^($chars+):($chars+)$/) {
> + my ($p1, $p2) = ($1, $2);
> + $parse_single_timespec->($p1, 24, \$matchall_hours, $hours_hash);
> + $parse_single_timespec->($p2, 60, \$matchall_minutes, $minutes_hash);
> + } elsif ($time_spec =~ m/^($chars)+$/) { # minutes only
> + $matchall_hours = 1;
> + foreach my $p (split(',', $event)) {
> + $parse_single_timespec->($p, 60, \$matchall_minutes, $minutes_hash);
> + }
> +
> + } else {
> + die "unable to parse calendar event\n";
> + }
> +
> + die "unable to parse calendar event - unused parts\n" if scalar(@parts);
> +
> + if ($matchall_hours) {
> + $h = '*';
> + } else {
> + $h = [ sort keys %$hours_hash ];
> + }
> +
> + if ($matchall_minutes) {
> + $m = '*';
> + } else {
> + $m = [ sort keys %$minutes_hash ];
> + }
> +
> + return { h => $h, m => $m, dow => [ sort keys %$dow_hash ]};
> +}
> +
> +sub compute_next_event {
> + my ($calspec, $last, $utc) = @_;
> +
> + my $hspec = $calspec->{h};
> + my $mspec = $calspec->{m};
> + my $dowspec = $calspec->{dow};
> +
> + $last += 60; # at least one minute later
> +
> + while (1) {
> +
> + my ($min, $hour, $mday, $mon, $year, $wday);
> + my $startofday;
> +
> + if ($utc) {
> + (undef, $min, $hour, $mday, $mon, $year, $wday) = gmtime($last);
> + $startofday = timegm(0, 0, 0, $mday, $mon, $year);
> + } else {
> + (undef, $min, $hour, $mday, $mon, $year, $wday) = localtime($last);
> + $startofday = timelocal(0, 0, 0, $mday, $mon, $year);
> + }
> +
> + $last = $startofday + $hour*3600 + $min*60;
> +
> + my $check_dow = sub {
> + foreach my $d (@$dowspec) {
> + return $last if $d == $wday;
> + if ($d > $wday) {
> + return $startofday + ($d-$wday)*86400;
> + }
> + }
> + return $startofday + (7-$wday)*86400; # start of next week
> + };
> +
> + if ((my $next = $check_dow->()) != $last) {
> + $last = $next;
> + next; # repeat
> + }
> +
> + my $check_hour = sub {
> + return $last if $hspec eq '*';
> + foreach my $h (@$hspec) {
> + return $last if $h == $hour;
> + if ($h > $hour) {
> + return $startofday + $h*3600;
> + }
> + }
> + return $startofday + 24*3600; # test next day
> + };
> +
> + if ((my $next = $check_hour->()) != $last) {
> + $last = $next;
> + next; # repeat
> + }
> +
> + my $check_minute = sub {
> + return $last if $mspec eq '*';
> + foreach my $m (@$mspec) {
> + return $last if $m == $min;
> + if ($m > $min) {
> + return $startofday +$hour*3600 + $m*60;
> + }
> + }
> + return $startofday + ($hour + 1)*3600; # test next hour
> + };
> +
> + if ((my $next = $check_minute->()) != $last) {
> + $last = $next;
> + next; # repeat
> + } else {
> + return $last;
> + }
> + }
> +
> + die "unable to compute next sync time\n";
"sync time" is probably leftover from when this was part of the
replication, but doesn't really fit here anymore
> +}
> +
> +1;
> --
> 2.11.0
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
