Re: desired bahavior for "email" interactivity?
Nathan Stratton Treadway <[email protected]>
| Newsgroups | gmane.comp.archivers.amanda.devel |
|---|---|
| Message-ID | <[email protected]> |
On Fri, Mar 17, 2017 at 15:51:41 -0400, Jean-Louis Martineau wrote: > On 17/03/17 08:45 AM, Nathan Stratton Treadway wrote: > > So, I guess the question is what's the desired behavior here? > > > > Off hand, I think I would expect 1) Amanda would send email every > > resend-delay seconds as long as it is waiting (for whatever reason), and > > 2) Amanda should always wait for input_file to change in some way from > > its initial state, but it shouldn't matter if that change is "creation" > > or "modification". > That's what I expect. Okay, great. Attached is a proof-of-concept patch to implement that behavior. As suggested in my earlier email, with this patch the program now saves an initial timestamp when it is first called and then waits for the file to be updated (i.e. either created or modified) before attempting to read it. Also, to avoid race conditions between the send_email and check_file functions, I moved all the logic to check the file type and permissions into the check_file function (which then, if necessary, saves the text of a warning message that will get included in the email generated by the send_email function). There are still situations where a change to the check-file file may result in two emails being generated the same second, one describing the state that existed before the change and one describing the new state. Presumably those situations won't occur too often in real-world usage, but to help the user understand that the two emails are actually different, the patch adds a line to the email showing information about the current state of the check-file. (In my testing, that information seemed useful even when there weren't double messages, since it made it clear what Amanda waiting for at each point in time...) (The patch also adds the changer name to the subject line of the generated email, since that seems useful to have menttioned, especially at sites with more than one changer in use.) I've attempted to follow the existing Perl style in the new code, but certainly would be interested in feedback on the Perl coding from any Perl gurus out there. Comments on "end-user" usability from anyone who's actually experienced the email interactivity in production would also be interesting... Nathan ---------------------------------------------------------------------------- Nathan Stratton Treadway - [email protected] - Mid-Atlantic region Ray Ontko & Co. - Software consulting services - http://www.ontko.com/ GPG Key: http://www.ontko.com/~nathanst/gpg_key.txt ID: 1023D/ECFB6239 Key fingerprint = 6AD8 485E 20B9 5C71 231C 0C32 15F3 ADCD ECFB 6239
email.pm_patch_20170326.diff
(text/x-diff, 4.4 KB)
--- perl/Amanda/Interactivity/email.pm_orig 2017-03-07 06:50:54.000000000 -0500
+++ perl/Amanda/Interactivity/email.pm 2017-03-26 16:18:58.216656034 -0400
@@ -101,10 +101,15 @@
my $mailer = getconf($CNF_MAILER);
my $subject;
+ if ($chg_name) {
+ $subject = "AMANDA VOLUME REQUEST ($chg_name):";
+ } else {
+ $subject = "AMANDA VOLUME REQUEST:";
+ }
if ($label) {
- $subject = "AMANDA VOLUME REQUEST: $label";
+ $subject .= " $label";
} else {
- $subject = "AMANDA VOLUME REQUEST: new volume";
+ $subject .= " new volume";
}
my $mailto;
@@ -135,16 +140,27 @@
if ($check_file) {
print {$fh} "or write the name of a new changer in '$check_file'\n";
print {$fh} "or write 'abort' in the file to abort the scan.\n";
- $self->{'check_file_mtime'} = 0;
- $self->{'check_file_ctime'} = 0;
- if (-e $check_file) {
- $self->{'check_file_mtime'} = (stat($check_file))->mtime;
- $self->{'check_file_ctime'} = (stat($check_file))->ctime;
- if (!-f $check_file) {
- print {$fh} "\nThe check-file '$check_file' is not a flat file.\n";
- } elsif (!-r $check_file) {
- print {$fh} "\nThe check-file '$check_file' is not readable.\n";
- }
+
+ # check_file_cb actually monitors the file's ctime and mtime
+ # timestamps separately, but it seems clearer for the email
+ # just to report the latest of the two to the user -- in
+ # practice the single date should be enough for the user to
+ # understand what Amanda is waiting for....
+ my $modtime = $self->{'check_file_mtime'};
+ if ( $self->{'check_file_ctime'} > $modtime ) {
+ $modtime = $self->{'check_file_ctime'};
+ }
+ if ( $modtime > 0 ) {
+ print {$fh} "\n(Waiting for the file to be modified after:\n" .
+ ctime($modtime) . ".)\n";
+ } else {
+ print {$fh} "\n(Waiting for the file to be created.)\n";
+ }
+
+ # if check_file_cb detected any warning message, include it
+ # here.
+ if ($self->{'check_file_message'}) {
+ print {$fh} $self->{'check_file_message'}
}
}
close $fh;
@@ -160,17 +176,33 @@
$self->{'check_file_src'} = undef;
if (-e $check_file) {
- $self->{'send_email_src'}->remove() if $self->{'send_email_src'};
- $self->{'send_email_src'} = undef;
my $check_file_mtime = (stat($check_file))->mtime;
my $check_file_ctime = (stat($check_file))->ctime;
if ($self->{'check_file_mtime'} < $check_file_mtime or
$self->{'check_file_ctime'} < $check_file_ctime) {
+ # The user has modified the file, so we stop
+ # the email callback. If we detect a problem with the
+ # file below, the email callback is restarted; otherwise
+ # this Interactivity call returns. (The caller may
+ # immediately invoke a new Interactivity call
+ # afterwards, if the user hasn't aborted and the caller
+ # still hasn't found the desired volume.... in which
+ # case the new call will start its own email callback.)
+ $self->{'send_email_src'}->remove() if $self->{'send_email_src'};
+ $self->{'send_email_src'} = undef;
+
+ # (save updated values, in case we don't return below.)
$self->{'check_file_ctime'} = $check_file_ctime;
$self->{'check_file_mtime'} = $check_file_mtime;
+ $self->{'check_file_message'} = undef;
- if (!-f $check_file || !-r $check_file) {
+ if (!-f $check_file) {
+ $self->{'check_file_message'} = "\nThe check-file '$check_file' is not a flat file.\n";
+ } elsif (!-r $check_file) {
+ $self->{'check_file_message'} = "\nThe check-file '$check_file' is not readable.\n";
+ }
+ if ($self->{'check_file_message'}) {
$send_email_cb->();
} else {
my $fh;
@@ -195,11 +227,28 @@
}
}
} else {
+ # the file doesn't currently exist
$self->{'check_file_mtime'} = 0;
+ $self->{'check_file_ctime'} = 0;
+ $self->{'check_file_message'} = undef
}
$self->{'check_file_src'} = Amanda::MainLoop::call_after($check_file_delay, $check_file_cb);
};
+ if ($check_file) {
+ # save the initial timestamps of the file, so we can detect
+ # when the user updates it later.
+ if (-e $check_file) {
+ $self->{'check_file_mtime'} = (stat($check_file))->mtime;
+ $self->{'check_file_ctime'} = (stat($check_file))->ctime;
+ $self->{'check_file_message'} = undef
+ } else {
+ $self->{'check_file_mtime'} = 0;
+ $self->{'check_file_ctime'} = 0;
+ $self->{'check_file_message'} = undef
+ }
+ }
+
$send_email_cb->();
if ($check_file) {
$check_file_cb->();