Remove unneeded namespacing in header files #921

RonEld · 2017-05-14T13:23:26Z

Remove the mbedtls namesapcing in the #include in header files
Resolves #857

hanno-becker · 2017-09-08T11:17:45Z

ChangeLog

+= mbed TLS x.x.x branch released xxxx-xx-xx
+
+Bugfix
+   * Fix namespacing in header files. REmove the `mbedtls` namespacing in


RonEld · 2017-10-01T14:12:50Z

@hanno-arm Thank you for your review
I fixed the typo. Please approve

andresag01 · 2017-10-02T17:53:28Z

include/mbedtls/net.h

 */

 #if !defined(MBEDTLS_DEPRECATED_REMOVED)
-#include "mbedtls/net_sockets.h"
+#include "net_sockets.h"
 #if defined(MBEDTLS_DEPRECATED_WARNING)
 #warning "Deprecated header file: Superseded by mbedtls/net_sockets.h"


Should we change this one as well?

@andresag01 Why do you think it shouldn't change? Although net.h is superseded, it can still be included.

This was a long time ago, so what I probably meant is that in the message for this warning there is a mbedtls/net_sockets.h. Do we want to make that net_sockets.h as well? Its a really minor thing and certainly not a stopper, more of a question. I will accept the PR.

I think in the message, it's more appropriate to keep it as is, as it is intended for human readers, and it's informative with the current wording

I personally think it's better to keep it as it is.

RonEld · 2018-06-21T11:23:31Z

I have merged development branch into this PR to resolve conflicts.
I have fixed typo in previous commit message.
@andresag01 @hanno-arm Please review

andresag01 · 2018-06-21T18:44:19Z

@RonEld: Thanks for looking into this again, but could you please rebase instead of creating a merge commit. For example, the diff now shows as if you had introduced a new config file and in general its not ideal to have these merge commits in the history.

Also, could you please create backports?

Remove the `mbedtls` namesapcing in the `#include` in header files Resolves issue Mbed-TLS#857

Fix typo in ChangeLog discovered in PR review

RonEld · 2018-06-24T14:01:01Z

@andresag01 I have removed the merge commit and rebased.
Please review

RonEld · 2018-06-24T14:30:32Z

backports created in #1779 and #1780 for mbedtls-2.1 and mbedtls-2.7 respectively

andresag01

Thanks for reworking the PR. In general, I am happy with the changes, but I have a question regarding the use of namespaced includes in the configs. Sorry about that, I should have spotted it in the earlier reviews.

andresag01 · 2018-06-26T18:46:42Z

include/mbedtls/net.h

@@ -1,7 +1,7 @@
 /**
 * \file net.h
 *
- * \brief Deprecated header file that includes mbedtls/net_sockets.h
+ * \brief Deprecated header file that includes net_sockets.h


Minor: Why did you remove mbedtls/ from this comment for example and not from the one below?

I don't really have a good answer.
Probably because in this comment, it's more reasonable that a human reader will read the file within the same folder, and in the warning message, the compilation will probably be from the root folder. I'm fine either way

andresag01 · 2018-06-26T18:57:23Z

configs/config-ccm-psk-tls1_2.h

@@ -83,6 +83,6 @@
 */
 #define MBEDTLS_SSL_MAX_CONTENT_LEN             512

-#include "mbedtls/check_config.h"
+#include "check_config.h"


The README.txt file in the configs directory states that there are two options to use these config files

Replace the default file include/mbedtls/config.h with the chosen one.
(Depending on your compiler, you may need to adjust the line with
#include "mbedtls/check_config.h" then.)

Define MBEDTLS_CONFIG_FILE and adjust the include path accordingly.

So in case (1) the user has to do nothing and just copy the file over as stated in the readme (actually, I think the information in brackets is no longer relevant). However, in the case of (2) the user will probably need to add ./configs to the include path as well as ./include/mbedtls (because this patch removes the namespacing bit). Also, note that without this change, case (1) will also work with the mbedtls/ prefix, its just not ideal and its already properly documented.

So I am not too sure that we should remove the namespacing from these config files. But if we do want to remove, we should at least fix the documentation accordingly.

I'll revert the changes in the configs

Dismissing review as there are pending questions.

RonEld · 2018-06-27T12:30:12Z

Added the "needs backports" label, as the backports are still pending reviews

Revert the changes in the `configs` folder to align with the `README.txt` file.

RonEld · 2018-07-01T07:45:38Z

@andresag01 I have reverted the changes in the configs folder, and added an mbedtls namespace in the config-no-entropy.h file as well. Please review

cc @hanno-arm

andresag01

@RonEld: Sorry to cause so much trouble over this, but please see my comment regarding the config-no-entropy. You could either leave the header as it was or fix the Mbed OS script for the next Mbed TLS release into that repo. However, to be honest I think this is a bit too much trouble to fix this issue, in reality I do not think this is causing anyone any trouble because the compiler is going to find the headers either way, its just a difference as to what it is going to search first...

andresag01 · 2018-07-02T18:34:56Z

configs/config-no-entropy.h

@@ -87,6 +87,6 @@
 /* Miscellaneous options */
 #define MBEDTLS_AES_ROM_TABLES

-#include "check_config.h"
+#include "mbedtls/check_config.h"


I think this might not be ideal. The issue is that this file is copied by some import script directly into the inc/mbedtls directory in Mbed TLS, so technically the mbedtls/ prefix is not needed.

Remove the `mbedtls` namespacing from the `config-no-entropy.h` file, as it is being imported to the include folder.

RonEld · 2018-07-03T13:09:32Z

@andresag01 I have removed the mbedtls namespacing as you requested.
I will update the backports once this PR is approved

simonbutcher

LGTM

simonbutcher · 2018-07-04T14:09:25Z

include/mbedtls/net.h

 */

 #if !defined(MBEDTLS_DEPRECATED_REMOVED)
-#include "mbedtls/net_sockets.h"
+#include "net_sockets.h"
 #if defined(MBEDTLS_DEPRECATED_WARNING)
 #warning "Deprecated header file: Superseded by mbedtls/net_sockets.h"


I personally think it's better to keep it as it is.

simonbutcher · 2018-07-05T08:35:58Z

@RonEld - Can you please backport the review feedback? #1779 and #1780 need the same fixes following the review of this PR.

RonEld · 2018-07-05T11:54:44Z

Done

simonbutcher · 2018-07-16T15:48:40Z

retest

simonbutcher added bug mbed TLS team labels May 23, 2017

hanno-becker reviewed Sep 8, 2017

View reviewed changes

Patater added needs-review Every commit must be reviewed by at least two team members, needs-work labels Sep 29, 2017

RonEld requested a review from andresag01 October 1, 2017 14:13

andresag01 reviewed Oct 2, 2017

View reviewed changes

RonEld mentioned this pull request Dec 21, 2017

adjust include paths #1225

Closed

simonbutcher added the needs-backports Backports are missing or are pending review and approval. label May 30, 2018

andresag01 previously approved these changes May 30, 2018

View reviewed changes

RonEld mentioned this pull request May 30, 2018

Automatically configure mbedtls_platform_zeroize under Windows #1644

Closed

RonEld force-pushed the 1301 branch from 7092762 to ae71561 Compare June 21, 2018 11:21

RonEld removed the needs-work label Jun 21, 2018

simonbutcher added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Jun 22, 2018

Remove unneeded namesapcing in header files

6fd941f

Remove the `mbedtls` namesapcing in the `#include` in header files Resolves issue Mbed-TLS#857

RonEld force-pushed the 1301 branch from ae71561 to fce77dc Compare June 24, 2018 13:59

Fix typo in ChangeLog

6332e36

Fix typo in ChangeLog discovered in PR review

RonEld force-pushed the 1301 branch from fce77dc to 6332e36 Compare June 24, 2018 14:00

RonEld added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Jun 24, 2018

This was referenced Jun 24, 2018

Backport 2.1: Remove unneeded namespacing in header files #1779

Merged

Backport 2.7: Remove unneeded namespacing in header files #1780

Merged

andresag01 reviewed Jun 26, 2018

View reviewed changes

andresag01 added needs-work and removed needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. labels Jun 27, 2018

RonEld added the needs-backports Backports are missing or are pending review and approval. label Jun 27, 2018

simonbutcher changed the title ~~Remove unneeded namesapcing in header files~~ Remove unneeded namespacing in header files Jun 29, 2018

Revert changes in the configs folder

13dfb4e

Revert the changes in the `configs` folder to align with the `README.txt` file.

RonEld added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Jul 1, 2018

andresag01 suggested changes Jul 2, 2018

View reviewed changes

RonEld added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Jul 3, 2018

Remove the namespacing from config-no-entropy.h

21f9afe

Remove the `mbedtls` namespacing from the `config-no-entropy.h` file, as it is being imported to the include folder.

RonEld added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Jul 3, 2018

andresag01 approved these changes Jul 3, 2018

View reviewed changes

simonbutcher approved these changes Jul 4, 2018

View reviewed changes

simonbutcher removed the needs-review Every commit must be reviewed by at least two team members, label Jul 4, 2018

andresag01 removed the needs-backports Backports are missing or are pending review and approval. label Jul 10, 2018

simonbutcher added the approved Design and code approved - may be waiting for CI or backports label Jul 16, 2018

simonbutcher added needs-ci Needs to pass CI tests and removed approved Design and code approved - may be waiting for CI or backports labels Jul 16, 2018

simonbutcher merged commit 21f9afe into Mbed-TLS:development Jul 20, 2018

RonEld mentioned this pull request Dec 30, 2018

Remove "mbedtls/" prefix in include file path #2319

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unneeded namespacing in header files #921

Remove unneeded namespacing in header files #921

RonEld commented May 14, 2017 •

edited

Loading

hanno-becker Sep 8, 2017

RonEld commented Oct 1, 2017

andresag01 Oct 2, 2017

RonEld May 30, 2018

andresag01 May 30, 2018

RonEld Jun 21, 2018

simonbutcher Jul 4, 2018

RonEld commented Jun 21, 2018

andresag01 commented Jun 21, 2018 •

edited

Loading

RonEld commented Jun 24, 2018

RonEld commented Jun 24, 2018

andresag01 left a comment

andresag01 Jun 26, 2018

RonEld Jun 27, 2018

andresag01 Jun 26, 2018

RonEld Jun 27, 2018

RonEld commented Jun 27, 2018

RonEld commented Jul 1, 2018

andresag01 left a comment

andresag01 Jul 2, 2018

RonEld commented Jul 3, 2018

simonbutcher left a comment

simonbutcher Jul 4, 2018

simonbutcher commented Jul 5, 2018

RonEld commented Jul 5, 2018

simonbutcher commented Jul 16, 2018

Remove unneeded namespacing in header files #921

Remove unneeded namespacing in header files #921

Conversation

RonEld commented May 14, 2017 • edited Loading

Choose a reason for hiding this comment

RonEld commented Oct 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonEld commented Jun 21, 2018

andresag01 commented Jun 21, 2018 • edited Loading

RonEld commented Jun 24, 2018

RonEld commented Jun 24, 2018

andresag01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonEld commented Jun 27, 2018

RonEld commented Jul 1, 2018

andresag01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonEld commented Jul 3, 2018

simonbutcher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbutcher commented Jul 5, 2018

RonEld commented Jul 5, 2018

simonbutcher commented Jul 16, 2018

RonEld commented May 14, 2017 •

edited

Loading

andresag01 commented Jun 21, 2018 •

edited

Loading