Page MenuHomeFreeBSD

traceroute6: Fix most of the warnings at the default WARNS level
ClosedPublic

Authored by shubh on Jul 9 2020, 7:06 PM.

Details

Summary

After removing WARNS?=3 from the Makefile, the following warnings came into the picture:

  • -Wmissing-variable-declarations The global variables should've been static, since traceroute6 only has one file.
  • -Wcast-align Fixing this error would've required creating many local variables on the stack since the error had many instances, hence -Wno-cast-align was added to the Makefile.
  • -Wincompatible-pointer-types-discards-qualifiers I created 2 new variables, to fix this problem, namely ipssp_inbypass and ipssp_outbypass
  • -Wshadow I gave new names to the variables which were being shadowed to deal with this error

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

shubh requested review of this revision.Jul 9 2020, 7:06 PM
shubh created this revision.
shubh edited the summary of this revision. (Show Details)
shubh edited the summary of this revision. (Show Details)

Could you please upload diffs with context? If you look at the examples on the wiki page of uploading patch files directly, they specify using -U99999.

Updated diff with context

This looks ok to me other than the feedback. Thanks for doing it!

usr.sbin/traceroute6/traceroute6.c
370 ↗(On Diff #74256)

I think the comments are not necessary. They also break style(9) since the lines are longer than 80 columns.

You don't need to include the array length in the definition, it should work to write char ipssp_inbypass[] = ....

I don't quite understand what the "ipssp" string means.

These strings should be defined under #if defined(IPSEC) && defined(IPSEC_POLICY_IPSEC).

807 ↗(On Diff #74256)

Looking at this more closely, it's fine to just reuse the top-level variables.

This makes me realize that the other use of getaddrinfo() above is missing a freeaddrinfo(res) call, so let's add that too.

  • Renamed the new variables, and wrapped them in #if defined
  • Removed the variable definitions in the source selection code, and reuse the top-level variables
  • added a freeaddrinfo(res) which was missing
  • Removed unnecessary comments

Thanks! I made one other comment, please fix and I'll do some testing and commit.

usr.sbin/traceroute6/traceroute6.c
370 ↗(On Diff #74598)

The "#if" should not be indented; see the other places where #ifdef etc. are used.

While here, could you convert other instances of

#ifdef IPSEC
#ifdef IPSEC_POLICY_IPSEC
...

to

#if defined(IPSEC) && defined(IPSEC_POLICY_IPSEC)

as you did here?

This revision is now accepted and ready to land.Jul 20 2020, 5:35 PM

Fixed indents and changed all instances of

#ifdef IPSEC
#ifdef IPSEC_POLICY_IPSEC

to

#if defined(IPSEC) && defined(IPSEC_POLICY_IPSEC)
This revision now requires review to proceed.Jul 20 2020, 6:32 PM
This revision is now accepted and ready to land.Jul 20 2020, 8:31 PM
usr.sbin/traceroute6/traceroute6.c
305 ↗(On Diff #74710)

You also need to remove one of the #endifs. Please make sure to at least test the build before uploading to phabricator in the future.