Page MenuHomeFreeBSD

Make arp return errors
ClosedPublic

Authored by bz on Jan 20 2019, 10:49 AM.

Details

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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 22687
Build 21787: arc lint + arc unit

Event Timeline

hselasky added inline comments.
sys/netinet/if_ether.c
344

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

This revision is now accepted and ready to land.Jan 25 2019, 8:26 PM

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
344

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

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.

arprequest_internal would be better.

+1

Rename arprequest_int() to arprequest_internal().

This revision now requires review to proceed.Feb 24 2019, 12:28 PM
bz marked an inline comment as done.Feb 24 2019, 12:31 PM

Generally looks good, one small comment inline.

sys/netinet/if_ether.c
432

I wonder if this log should be rate-limited?

bz marked an inline comment as done.Feb 24 2019, 1:59 PM
bz added inline comments.
sys/netinet/if_ether.c
432

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 added inline comments.
sys/netinet/if_ether.c
432

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

This revision is now accepted and ready to land.Feb 24 2019, 2:31 PM
This revision was automatically updated to reflect the committed changes.
bz marked an inline comment as done.