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

Allow builds with -std=c99 on LTS branch 2.7 #3656

Closed
wants to merge 3 commits into from
Closed

Allow builds with -std=c99 on LTS branch 2.7 #3656

wants to merge 3 commits into from

Conversation

gufe44
Copy link
Contributor

@gufe44 gufe44 commented Sep 9, 2020

Includes (partial) backports of #1198, #3190, #3421, #3422 and #3571.

@daverodgman daverodgman added the needs-review Every commit must be reviewed by at least two team members, label Sep 10, 2020
@yanesca yanesca added the needs-reviewer This PR needs someone to pick it up for review label Oct 2, 2020
@daverodgman daverodgman self-assigned this Oct 7, 2020
@daverodgman daverodgman self-requested a review October 7, 2020 16:29
if( unmet_dependencies[unmet_dep_count] == NULL )
{
mbedtls_fprintf( stderr,
"FATAL: Out of memory\n" );
mbedtls_exit( MBEDTLS_EXIT_FAILURE );
}
for( j = 0; j < len; j++ )
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be simpler to call strncpy here

@daverodgman daverodgman added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Oct 8, 2020
gufe44 added 3 commits October 8, 2020 11:55
Do this by including strings.h and defining feature test macros _POSIX_C_SOURCE, _XOPEN_SOURCE, _GNU_SOURCE, _BSD_SOURCE and _NETBSD_SOURCE

Signed-off-by: gufe44 <[email protected]>
Signed-off-by: gufe44 <[email protected]>
@daverodgman daverodgman added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Oct 8, 2020
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes are ok, except that the _xxx_SOURCE macros should only be defined if they aren't already defined.

Please add -std=c99 -pedantic to test_build_opt in all.sh, as is already the case in mbedtls-2.16 and development, to avoid future regressions.

@@ -44,6 +44,11 @@
* **********
*/

#if defined(__linux__)
/* Ensure that syscall() is available even when compiling with -std=c99 */
#define _GNU_SOURCE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As reported in #3432, _GNU_SOURCE should not be redefined if it's already defined.

This also applies to _POSIX_C_SOURCE, _XOPEN_SOURCE and any similar macro.

@gilles-peskine-arm gilles-peskine-arm added component-platform Portability layer and build scripts needs-work and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Oct 13, 2020
@gufe44
Copy link
Contributor Author

gufe44 commented Oct 30, 2020

Unfortunately, for personal reasons I won't be able to continue to work on the PR. You can close the PR now and resurrect the branch at any time.

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As with #3658 (review), I don't think these changes are correct, because they require a more recent POSIX version than what the long-time-support branch originally required.

@chris-jones-arm
Copy link
Contributor

I agree with the above. This is being closed as these changes would not be compatible with older systems on the LTS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-platform Portability layer and build scripts needs-work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants