Page MenuHomeFreeBSD

ping: fix alignment errors
ClosedPublic

Authored by jansucan on Aug 19 2019, 6:45 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 20, 2:25 PM
Unknown Object (File)
Sat, Mar 16, 10:08 AM
Unknown Object (File)
Sat, Mar 16, 10:08 AM
Unknown Object (File)
Sat, Mar 16, 10:08 AM
Unknown Object (File)
Sat, Mar 16, 10:08 AM
Unknown Object (File)
Sat, Mar 16, 10:04 AM
Unknown Object (File)
Dec 31 2023, 1:22 AM
Unknown Object (File)
Dec 12 2023, 12:43 AM
Subscribers

Details

Summary

Fix alignment errors

This fixes -Wcast-align errors when compiled with WARNS=6.

Submitted by: Ján Sučan <sucanjan@gmail.com>
Sponsored by: Google LLC (Google Summer of Code 2019)

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 26051
Build 24595: arc lint + arc unit

Event Timeline

This change breaks the build on powerpc, powerpc64, and sparc64:

/home/asomers/freebsd/base/head/sbin/ping/ping.c: In function 'pr_pack':
/home/asomers/freebsd/base/head/sbin/ping/ping.c:1144: warning: signed and unsigned type in conditional expression

If the line 1144 is this line

if (icp.icmp_type == icmp_type_rsp) {

then it's interesting because both of the variables are u_char. Could you please provide me more info from the compiler?

That's the whole of the compiler's error output. If you want to reproduce it locally, you should be able to do it with env ARCH=powerpc TARGET_ARCH=powerpc make buildworld. I recommend adding -DNO_CLEAN if you do it more than once. And don't forget the appropriate -j X, or you'll be waiting all day.

Fix the warning about signed and unsigned type in conditional
expression.

sbin/ping/ping.c
1145

Would this be more clearly written MIN((int)sizeof(icp), cc)?
And since cc is initialized from the output of recvmsg, shouldn't it be of type ssize_t?

sbin/ping/ping.c
1145

And since cc is initialized from the output of recvmsg, shouldn't it be of type ssize_t?

That's true. Current use of data types in the code is less appropriate than it should be. I will try to fix this in another diff.

Use MIN() instead of a custom conditional expression.

sbin/ping/ping.c
1145

After doing some experiments it seems that the least error prone way would be to type keep the type casting in (int)sizeof(icp). Changing data type of the variable for recvmsg's return value is OK. The problem is with cc argument to pr_pack. Changing it from int to ssize_t doesn't solve warning about comparing integers with different signs. Changing it from int to size_t (it's a size of a data buffer) starts avalanche of warnings about different signs. If one is corrected, another two appear. Better time for fixing this would be when the code is much more structured so it's much easier to see whether a variable really needs to hold negative values or not.

sbin/ping/ping.c
1145

True, a size cast would be needed if cc were ssize_t. But that's still better than the current situation which involves both a sign cast and a size cast (implicitly, when assigning from recvmsg).

sbin/ping/ping.c
1145

Can I change the data type in this diff, or should I create a separate one?

sbin/ping/ping.c
1145

It should go into this diff. I don't see any reason to postpone.

Change data type of the variable for recvmsg's return value from int to ssize_t.

asomers requested changes to this revision.Aug 23 2019, 4:27 PM
asomers added inline comments.
sbin/ping/ping.c
898–899

I don't think there's any reason to change the type of n, too.

This revision now requires changes to proceed.Aug 23 2019, 4:27 PM

Data type of variable n for return value of pselect doesn't need to be changed.

sbin/ping/ping.c
898–899

Fixed.

This revision needs to be rebased since the changes to -n and -H.

Rebase to the git master branch.

This revision is now accepted and ready to land.Aug 23 2019, 10:02 PM
This revision was automatically updated to reflect the committed changes.