[pve-devel] [PATCH 1/2] CalendarEvent.pm: implement parser/utils for systemd like calender exents

Wolfgang Bumiller w.bumiller at proxmox.com
Wed May 17 11:59:02 CEST 2017


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 <dietmar at 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




More information about the pve-devel mailing list