Page MenuHomeFreeBSD

arp.c: use getaddrinfo(3) instead of gethostbyname(3)
AcceptedPublic

Authored by luthramihir708_gmail.com on Tue, Sep 10, 4:36 PM.

Details

Reviewers
hrs

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 26504
Build 24909: arc lint + arc unit

Event Timeline

linimon retitled this revision from use getaddrinfo(3) instead of gethostbyname(3) to arp.c: use getaddrinfo(3) instead of gethostbyname(3).Thu, Sep 12, 1:40 AM
hrs requested changes to this revision.Thu, Sep 12, 3:48 AM
hrs added a subscriber: hrs.
hrs added inline comments.
usr.sbin/arp/arp.c
274–275

I suggest to rename reply to sin in addition to the other changes.

275–292

Use memset() instead of bzero() for userland utilities.

276

hints.ai_family must be used here.

281

xo_errx() does not return.

295

An assertion for res->ai_addrlen == sizeof(struct sockaddr_in) might be worth being added before memcpy(). And I personally think that an warning should be displayed when getaddrinfo() returns multiple addresses.

This revision now requires changes to proceed.Thu, Sep 12, 3:48 AM
  1. Updating D21589: arp.c: use getaddrinfo(3) instead of gethostbyname(3) #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment. #
  4. If you intended to create a new revision, use:
  5. $ arc diff --create

1)changed variable name 'reply' to 'sin'
2)use memset(3) instead of bzero(3)
3)bug fix: AF_INET flag supplied in hints.ai_family instead of hints.ai_flags
4)replaced xo_errx(3) with xo_warnx(3) when host wasn't resolved
5)add assrtion when getaddrinfo doesn't return an ipv4 address
6)add warning when getaddrinfo returns multiple addresses

luthramihir708_gmail.com marked 4 inline comments as done.Thu, Sep 12, 6:56 AM
luthramihir708_gmail.com added inline comments.
usr.sbin/arp/arp.c
276

Sorry that was by mistake. Thanks

295

If getaddrinfo(3) returns multiple addresses, should I loop to check if any of them is ipv4. If I am not wrong, arp is meant only for ipv4 right?
Thanks for the review :)

295

I was thinking if I am specifying family as AF_INET, it shouldn't return any sockaddr_in6. Shouldn't it be fine without check? Can ai_addr contain anything except sockaddr_in when family is AF_INET?

luthramihir708_gmail.com marked 3 inline comments as done.Thu, Sep 12, 8:49 AM
  1. Updating D21589: arp.c: use getaddrinfo(3) instead of gethostbyname(3) #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment. #
  4. If you intended to create a new revision, use:
  5. $ arc diff --create

Removed unecessary warning

hrs added inline comments.Sun, Sep 15, 12:54 AM
usr.sbin/arp/arp.c
285

I think "%s: %s" should be enough here as in the original version. gai_strerror() will put a description such as "Name does not resolve" when EAI_NONAME, for example.

295

What I suggested was use of assert(), not a conditional, for debugging purpose. This is not directly related to AF_INET6 or others which getaddrinfo(3) can return (and you seem to miss a possibility that getaddrinfo(3) can return other than AF_INET{,6}). The reason why adding assert(res->ai_addrlen == sizeof(sin)) is that ai_addrlen is not constant. A buffer length check in C language must be conservative---even if someone may mistakenly remove hints.ai_family = AF_INET in the future, the assert() eliminates possibility of buffer overrun.

296

Please remove the trailing whitespaces of this line. When multiple addresses are found, I think arp(8) should abort (i.e. this function returns NULL) because it is quite likely against the sysadmin's intention.

Made corrections as suggested.

luthramihir708_gmail.com marked 3 inline comments as done.Sun, Sep 15, 4:32 AM
usr.sbin/arp/arp.c
295

Got it. I will remember to consider such scenarios too. Thanks.

hrs accepted this revision.Sat, Sep 21, 1:54 PM

Looks good to me.

This revision is now accepted and ready to land.Sat, Sep 21, 1:54 PM