Fix an incorrect hyphen removal due to a double linefeed

[tonvoon]

Given a message of "LONGOUTPUT\n\n blah.", the plugin_exit() message gave: "MONITORING-PLUGIN-FUNCTIONS-01 CRITICALLONGOUTPUT\n\n blah.\n", which was wrong.

This was located as the regex in this line:

$output .= " - " unless $message =~ /^\s*\n/mxs;

I have amended the line to:

$output .= " - " unless $message =~ /^\s*[\n\r]/;

which kinda makes sense - it means if the message starts with (optional) spaces and then a linefeed, ignore the hyphen. The previous modifiers of /mxs started to match other parts of the string.

Have added a test case to cover this and also update the test to check for the precise output (rather than the looser regex check).

[dermoth]

Hi Ton!

I tried to make sense of this... While the fix is sound, long output seem to be a somewhat undocumented feature of N::P. In particular it wouldn't respect the standard v3 plugin format (I'm not sure how Nagios & others cope with this though, maybe it's OK regardless...)

The format since v3 is defined as:

TEXT OUTPUT | OPTIONAL PERFDATA
LONG TEXT LINE 1
LONG TEXT LINE 2
...
LONG TEXT LINE N | PERFDATA LINE 2
PERFDATA LINE 3
...
PERFDATA LINE N

Yet the first perfdata would start to appear at PERFDATA LINE 2, with a possibly confusing output if there is a trailing newline.

Furthermore there is no room for a short status text (ex something that can easily fit in an sms or other short notification message).

So why not instead split on the first \r?\n, take first part as short output, append perfdata, followed by the remaining output, stripped (so there is no extra newlines at the end)? In an ideal world there could be facilities for extra output/perfdata lines but for now any plugin could append it itself in the extra output if desired (AFAIK N::P::Performance can even be leveraged for that, it just won't be automatic).

If long output facilities are added in the future it could allow either long output/perfdata or newlines in short output, not both, for a smooth transition.

I'm also a bit worried about allowing \r\n... wouldn't it be be safer to either fail or just strip any \r and not worry about its possible interpretation down the line? (especially having a mix of \r\n and \n line endings which always cause trouble from my experience, and we probably don't want to add any detection/handling code just to try making that consistent).

[dermoth]

P.S.: Maybe a good opportunity to dust-off my Perl... Though it will definitively give me the impression of being stuck in an infinite loop... Bash, Perl, Python, Bash, Perl, ... :D

[tonvoon]

Hi Thomas!

Good question. I hadn't considered the perfdata angle of it. It looks like Sven made changes around the line, but my biggest beef is that the testcases weren't updated to cover what was trying to be achieved. The key will be adding further test cases and then code to that.

Could you add tests for what you expect?

[sni]

Looking at the history of that line, we had /^[ \f\r\t\w]*\n/ in https://github.com/monitoring-plugins/monitoring-plugin-perl/commit/94bb1dec4d9c8fec5e4aa51f9f8969be3a11528e

Then we had /^\h*\R/ and after that /^\s*\R/mxs

which did not work with < 5.8 then we then had /^\s*\n/mxs with https://github.com/monitoring-plugins/monitoring-plugin-perl/commit/02f2f8a42fb976ea04ae8be54b5b027e757cefc3

So looking at the number of changes for this little regex, i totally agree and we really need more testcases here.