Make arp return errors

[bz]

Summary

arprequest() is a void function and in case of error we simply
return without any feedback. In case of any local operation
or *if_output() fails no feedback is send up the stack for the
packet which triggered the arp request to be sent.
arpresolve_full() has three pre-canned possible errors returned
(if we have not yet sent enough arp requests or if we tried
often enough without success) otherwise "no error" is returned.

Make arprequest() an "internal" function arprequest_int() which
does return a possible error to the caller. Preserve arprequest()
as a void qrapper function for external consumers.
In arpresolve_full() add an extra error checking. Use the
arprequest_int() function and only return an error if non of the
three ones (mentioend above) have been set already.

This will return possible errors all the way up the stack and
allows functions and programs to react on the send errors rather
than leaving them in the dark. Also they might get more detailed
feedback of why packets cannot be sent and they will receive it
quicker.

Test Plan

Someone will tell me why this is a really bad idea.

Introduced selective ip_output() errors and observed that they
were reported.

[• hselasky]

sys/netinet/if_ether.c

345 ↗

(On Diff #53035)

arprequest_sub() is maybe a better name than xxx_int() ?

[bz]

I know the initial emails were eaten by phabricator. I am still hoping someone would tell me why this is a really bad idea... or not. @karels @gnn ?

sys/netinet/if_ether.c

345 ↗

(On Diff #53035)

I should spell it out like in_pcbinshash_internal() if_attach_internal() or if_detach_internal().

[• karels]

I agree that arprequest_int is rather opaque. At first I thought that is was just the version that returns int, which is not useful. arprequest_internal would be better. Otherwise, I have no objection; returning any errors seems like a good thing. And yes, I did not see any of the earlier (presumed) emails for this review.

[ae]

In D18904#405557, @karels wrote:

arprequest_internal would be better.

+1

[bz]

Rename arprequest_int() to arprequest_internal().

[• karels]

Generally looks good, one small comment inline.

sys/netinet/if_ether.c

432 ↗

(On Diff #54290)

I wonder if this log should be rate-limited?

[bz]

sys/netinet/if_ether.c

432 ↗

(On Diff #54290)

They all are:

164 #define ARP_LOG(pri, ...)       do {                                    \
165         if (ppsratecheck(&arp_lastlog, &arp_curpps, arp_maxpps))        \
166                 log((pri), "arp: " __VA_ARGS__);                        \
167 } while (0)

[• karels]

sys/netinet/if_ether.c

432 ↗

(On Diff #54290)

Never mind, I see that ARP_LOG *is* rate-limited.