Multiping!

[cron2]

Here's the PR as discussed in issue #52

I have rewritten it to add "-a", keep the calling convention for ping_host_add() and update the .pod documentation.

I'm not particularily attached to implementation details - this is a suggested way to implement it, which works for me :-) - but I can rewrite it (options, names, functions, ...) to better suit the project style.

If it were my decision, I would make "-a" the default, because that is what I use - "ping that thing, with whatever you have, all of it!". So there could be an option, like "-1", to turn it off and return to "only the first hit is pinged". But this is really fine tuning. Having the default "behaviour as before" with an extra "-a" to get "all targets" also works for me (and there's good arguments for not changing user visible behaviour).

As to the patch itself: there is two commits in the branch, one is the "multiping" functionality. The second one is something I find useful: if I have two hosts "heise.de" in the noping output, it's not easy to see whether v4 or or v6 ping is failing, so I modify the hostname in the liboping obj and attach a "/v4" or "/v6" to it, so it is more easily seen. This is totally independent, and the multiping change does not need it.

[xtaran]

@cron2:

if I have two hosts "heise.de" in the noping output, it's not easy to see whether v4 or or v6 ping is failing, so I modify the hostname in the liboping obj and attach a "/v4" or "/v6" to it, so it is more easily seen.

Cool idea! One nitpick though:

This works for the common 1×A and 1× AAAA records. There are though cases like e.g. weebly.com with two A records:

$ host -t A weebly.com
weebly.com has address 74.115.50.109
weebly.com has address 74.115.50.110

In such cases, I suggest to directly add a slash and then the actual IP address instead of just /v4 or /v6.

Not sure if it makes sense to always directly add the IP address if multiple A and/or AAAA records are present as /v4 and especially /v6 is much shorter an according IP address and hence also way easier to read and recognize.

@octo: Any news on this? I'd really would like to see this in the next Debian release. See https://release.debian.org/bullseye/freeze_policy.html for the planned freeze dates and migration delays.

[cron2]

@xtaran

One nitpick though:

This works for the common 1×A and 1× AAAA records. There are though cases like e.g. weebly.com with two A records:

Good point. In a "normal" terminal, there's definitely sufficient space to fully append the IPv4/IPv6 address(es) to the hostname, and it's not hard to do.

Not sure if it makes sense to always directly add the IP address if multiple A and/or AAAA records are present as /v4 and especially /v6 is much shorter an according IP address and hence also way easier to read and recognize.

Yeah. So either another option (which I do not like so much), or "if there is only a single v4/v6 address, append literal /v4, and if there is more than one, append the whole address". This is a bit more complicated to implement (you need two passes through the addrinfo to first get the number, and then create the hostnames). Mmmh. I need to play with this... :-)

@octo: Any news on this? I'd really would like to see this in the next Debian release. See https://release.debian.org/bullseye/freeze_policy.html for the planned freeze dates and migration delays.

yeah, some feedback from @octo on what he would like to merge would help me to decide what to code...

[barak]

[cron2]

Hi,

On Wed, Sep 30, 2020 at 08:36:10AM -0700, Florian Forster wrote:

I'm quite happy with this code, thank you @cron2! I'm a bit unsure about the API, specifically how useful it is to specify the number of hosts. In most cases, I'd expect users to either want one or "all". That leads to the two most common cases being 1 and 0, where "multi(1)" means "not actually multi" and "multi(0)" means "all hosts". I find this a bit unintuitive.

Turning this into a boolean for "all?" or "just the first one?" is easy.

I suggest to change the new ping_host_add_multi function to always add all IPs. Internally, we can continue to use the "number of entries" API, which should keep the required changes to a minimum. Does that sound reasonable?

This confuses me a bit. The point of adding this was that the caller does not have to decide "which function do I need to call?" but that a (new) caller could always call ping_host_add_multi(), and use an option to decide what to add.

Otherwise, you end up with code like this

if (opt_give_me_all) { n = ping_host_add_multi(ping, host); } else { n = ping_host_add(ping, host); }

in the caller (oping.c), which I do not find very pretty... maybe I'm misunderstanding your intention?

@@ -855,6 +856,10 @@ static int read_options (int argc, char *argv) / {{{ */ break; }

• 	case 'a':
  

• 		opt_max_hosts = -1;	/* all */
  

The documentation specifies "all" as zero, this code here uses -1.

Oups :-)

The actual code does not care "anything <= 0 means 'all'".

But yeah, this needs fixing.

gert

"If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany [email protected]