Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

issue #4579 #4877

Merged
merged 1 commit into from Jan 24, 2017
Merged

issue #4579 #4877

merged 1 commit into from Jan 24, 2017

Conversation

@skolot
Copy link
Contributor

@skolot skolot commented Jan 11, 2017

Short description

in sunos byte order macroses LE_IN/BE_IN expected pointer to integer as argument

issue #4579

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added regression tests
  • added unit tests
  • checked that this code was merged to master
@pieterlexis
Copy link
Member

@pieterlexis pieterlexis commented Jan 24, 2017

@rgacogne could you review?

@pieterlexis pieterlexis merged commit dfcb977 into PowerDNS:master Jan 24, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -59,23 +59,22 @@
#define le64toh(x) OSSwapLittleToHostInt64(x)
#endif

// for illumos
#ifdef BE_64
#ifdef __sun

This comment has been minimized.

@jeffpc

jeffpc Jan 24, 2017
Contributor

This will also be true on illumos, but in general isn't it considered bad form to check for specific systems instead of checking for features?


#define htobe16(x) BE_16(x)
#define htole16(x) LE_16(x)
#define be16toh(x) BE_IN16(x)
#define le16toh(x) LE_IN16(x)
#define be16toh(x) BE_IN16(&(x))

This comment has been minimized.

@jeffpc

jeffpc Jan 24, 2017
Contributor

I don't understand why these use {BE,LE}_INXX instead of {BE,LE}_XX...

@Habbie
Copy link
Member

@Habbie Habbie commented Mar 1, 2017

@jeffpc is any followup work required from your post-merge review of this?

@rgacogne
Copy link
Member

@rgacogne rgacogne commented Mar 2, 2017

For the record, during the last discussion it was pointed out that the {BE,LE}_INXX function-like macros are documented as accepting "unaligned values" while the {BE,LE}_XX ones are not.

@jeffpc
Copy link
Contributor

@jeffpc jeffpc commented Mar 2, 2017

IIRC, the change will work; it's just not necessarily the cleanest. I don't know threshold in pdns for code cleanups/improvements.

I don't understand the asymmetry between the hto{be,le}XX and {be,le}XXtoh. Why is one using the IN versions while the other isn't? Also, I just pointed out the use of a __sun guard. Both are cleanliness issues IMO and nothing more.

@rgacogne
Copy link
Member

@rgacogne rgacogne commented Mar 2, 2017

I don't know threshold in pdns for code cleanups/improvements.

I'll be happy to do as most cleanup as possible once I'm sure it's not breaking anything :)

I don't understand the asymmetry between the hto{be,le}XX and {be,le}XXtoh. Why is one using the IN versions while the other isn't?

I'm not sure that's the reason, but the host to network are far less likely to be used on non-aligned data than the network to host ones.

Also, I just pointed out the use of a __sun guard.

Agreed, we should fix that no matter what we decide on the other issue.

@Habbie
Copy link
Member

@Habbie Habbie commented Mar 2, 2017

@jeffpc want do a PR with your suggested cleanups? In the comments of this closed one we will all forget.

pieterlexis added a commit to pieterlexis/pdns that referenced this pull request May 9, 2017
@pieterlexis pieterlexis mentioned this pull request May 9, 2017
4 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.