From 301a3564e6b7cf716b75440c5333e688ea814873 Mon Sep 17 00:00:00 2001
From: Tom Ryder <tom@sanctum.geek.nz>
Date: Thu, 24 May 2018 13:01:21 +1200
Subject: Fail with config ->errstr() when undef read

The Config::Tiny parent class for Monitoring::Plugin::Config does not
necessarily raise an exception in $@/$EVAL_ERROR on a failed call to its
->read() method. Indeed, it does not in most cases, at least in the most
recent Config::Tiny.

If a file does not exist or for whatever other reason cannot actually be
read--such as permissions problems--Monitoring::Plugin ends up reporting
a "Missing config section" to the caller, which is misleading.

This information is available from the ->errstr() method of the base
class, however, and an inspection of the code for the ->read() method in
the parent class suggests the correct approach is to look for a return
of `undef` from the ->read() method, and use the return value of
->errstr() as the reported error for ->_die(). This commit adds such a
check.

To reproduce, given the following plugin `mptest`:

    use strict;
    use warnings;
    use Monitoring::Plugin qw(%ERRORS);
    my $mp = Monitoring::Plugin->new(
    	usage => '',
    );
    $mp->getopts;
    $mp->plugin_exit($ERRORS{OK}, 'Done!');

Running it without options of course works, like so:

    $ perl mptest
    MPTEST OK - Done!

However, prior to this patch, if we specify --extra-opts with a
nonexistent filename, we get a misleading error:

    $ perl mptest --extra-opts=@/nonexistent
    Invalid section 'mptest' in config file '/nonexistent'

With this patch included, the error is more accurate and helpful:

    $ perl mptest --extra-opts=@/nonexistent
    Failed to open file '/nonexistent' for reading: No such file or directory
---
 lib/Monitoring/Plugin/Getopt.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

(limited to 'lib/Monitoring/Plugin')

diff --git a/lib/Monitoring/Plugin/Getopt.pm b/lib/Monitoring/Plugin/Getopt.pm
index c8033d0..1810fde 100644
--- a/lib/Monitoring/Plugin/Getopt.pm
+++ b/lib/Monitoring/Plugin/Getopt.pm
@@ -260,7 +260,9 @@ sub _load_config_section
 
   my $Config;
   eval { $Config = Monitoring::Plugin::Config->read($file); };
-  $self->_die($@) if ($@); #TODO: add test?
+  $self->_die($@) if ($@);
+  defined $Config
+      or $self->_die(Monitoring::Plugin::Config->errstr);
 
   # TODO: is this check sane? Does --extra-opts=foo require a [foo] section?
   ## Nevertheless, if we die as UNKNOWN here we should do the same on default
-- 
cgit v1.2.3-74-g34f1