Skip to content

Commit

Permalink
check_dig: stick with integer devision
Browse files Browse the repository at this point in the history 
This change saves us from having to link check_dig against "libm"
  • Loading branch information
@waja
waja committed Jan 30, 2014
1 parent 2aa6ce2 commit e33ecc8
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion plugins/check_dig.c
View file
Expand Up @@ -90,7 +90,7 @@ main (int argc, char **argv)
usage_va(_("Could not parse arguments"));

/* dig applies the timeout to each try, so we need to work around this */
int timeout_interval_dig = ceil((double) timeout_interval / (double) number_tries);
int timeout_interval_dig = timeout_interval / number_tries + number_tries;

/* get the command to run */
xasprintf (&command_line, "%s @%s -p %d %s -t %s %s %s +tries=%d +time=%d",
Expand Down

7 comments on commit e33ecc8

@abrist
Copy link
Contributor

@abrist abrist commented on e33ecc8 Jan 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holger made a good point about this change:
nagios-plugins/nagios-plugins@05a63d8
ceil may be more correct as the plugin will still alarm once it reaches the global timeout value.

@weiss
Copy link
Member

@weiss weiss commented on e33ecc8 Jan 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that we're using timeout_interval / number_tries + number_tries (while your interim solution didn't add number_tries to the result). Yes, the results will often be, err, "less correct" compared to the floating point division, but I think this might be good enough for us. Especially as we didn't add a fancy --retries flag like you did :-)

But thanks for the hint!

@willixix
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

long long timeout_interval_ll = (timeout_interval<<8);

timeout_interval_ll = timeout_interval_ll / ( number_tries<<8);

int timeout_interval_dig = (timeout_interval_lg >>8) + number_tries;

@monitoring-user
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hehe, thanks, but personally I'd strongly prefer the floating point division then :-P

@abrist
Copy link
Contributor

@abrist abrist commented on e33ecc8 Jan 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be good enough, but most timeout values other than those that produce 0 from the division (less than 3) will cause dig to only timeout twice with the patch. I think the disagreement is philosophical at this point: I would rather allow the user to control the behavior - if they have a slow server or an intermittent one, they may wish for more retries etc. So we will stick with floating point division, though this does work just fine as well and escapes having to pull in the math libraries.

Cheers all.

@willix. no bitwise thank you, though it is clever :)

@weiss
Copy link
Member

@weiss weiss commented on e33ecc8 Jan 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I was inclined to hardcode +tries=1 on the dig(1) command line. While the dig(1) command happens to implement retries by default, most of our plugins don't, because users might want to notice if some fraction of queries fail, and because Nagios & friends support "soft states" if users don't want to be notified in such cases. I'd be surprised if the original check_dig author consciously relied on these dig(1) semantics.

And yes, maybe the disagreement is "philosophical": I do not at all believe that more configuration switches are necessarily better. Only those that might actually be useful are good :-) But I'll agree that there are many cases where you could argue one way or the other.

Either way, if there's just one more comment on this single line of code, I'll revert to the floating point division, which I agree is totally fine as well ;-)

@waja
Copy link
Member Author

@waja waja commented on e33ecc8 Jan 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still love each all! ;)

Please sign in to comment.