Multiping! #56
Multiping! #56
Conversation
This renames the ping_host_add() function to ping_host_add_multi() and adds an extra argument to it, specifying how many addresses to add from what getaddrinfo() returns. ping_host_add() is now a small wrapper that calls "ping_host_add_multi()" with "max = 1" to get the previous behaviour. The callers (oping/noping) default to "max = 1" unless a new option, "-a" is specified. In that case, all IPv4+IPv6 addresses returned by getaddrinfo() are added (-4 / -6 restricts this to "all IPv4 / IPv6 addresses", respectively). Technically, the change is fairly straightforward: ping_host_add() (now ping_host_add_multi()): - new argument "max_hosts" - move the allocation of "ph" to inside the "for (ai_ptr...)" loop - move adding of the found object to the "obj->" chain inside the loop - do not exit the loop until "enough" hosts have been found (or the list of results is exhausted) - return the number of hosts added (0..N) the callers need to be adjusted to learn the number added, because otherwise the graphical display in "noping" does not get extra lines. Signed-off-by: Gert Doering <[email protected]>
With the "multiping" patch, you end up seeing a hostname multiple times in the "noping" graphical display, and it's not easy to see right away whether the IPv4 or IPv6 address has loss. So, extend hostnames with "/v4" or "/v6" to make this visible. No option to turn this on or off has been implemented so far, but it could be made dependant on "-4"/"-6" (no suffix needed) or on an extra option. Signed-off-by: Gert Doering <[email protected]>
Cool idea! One nitpick though: This works for the common 1×A and 1× AAAA records. There are though cases like e.g. In such cases, I suggest to directly add a slash and then the actual IP address instead of just Not sure if it makes sense to always directly add the IP address if multiple A and/or AAAA records are present as @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. |
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.
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... :-)
yeah, some feedback from @octo on what he would like to merge would help me to decide what to code... |
|
|
There was a problem hiding this comment.
Hi all, sorry for the silence!
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.
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?
Thanks again and best regards,
—octo
| @@ -855,6 +856,10 @@ static int read_options (int argc, char **argv) /* {{{ */ | |||
| break; | |||
| } | |||
|
|
|||
| case 'a': | |||
| opt_max_hosts = -1; /* all */ | |||
There was a problem hiding this comment.
The documentation specifies "all" as zero, this code here uses -1.
|
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]
|
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.