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

Fix missing header when defining MBEDTLS_DEBUG #6045

Closed

Conversation

@fwh-dc
Copy link
Contributor

@fwh-dc fwh-dc commented Oct 6, 2020

Building curl fails when defining MBEDTLS_DEBUG for mbedtls version 2.16.x because of a missing header.

@bagder
Copy link
Member

@bagder bagder commented Oct 6, 2020

and this header is also present in somewhat older versions?

@fwh-dc
Copy link
Contributor Author

@fwh-dc fwh-dc commented Oct 6, 2020

It is present from version 2.0. I can add a compile time check for MBEDTLS_VERSION_MAJOR >= 2 ?

Copy link
Contributor

@emilengler emilengler left a comment

I am not into this library but according to your description the definition is probably a CPP macro so this doesn't matter

@@ -46,6 +46,8 @@
#include <mbedtls/ctr_drbg.h>
#include <mbedtls/sha256.h>

#include <mbedtls/debug.h>
Copy link
Contributor

@emilengler emilengler Oct 6, 2020

Choose a reason for hiding this comment

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

Shouldn't you add a #ifdef MBEDTLS_DEBUG?

Copy link
Contributor Author

@fwh-dc fwh-dc Oct 6, 2020

Choose a reason for hiding this comment

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

From the code it looks like the idea is that you manually "#define" instead of the "#undef" in

#undef MBEDTLS_DEBUG

Assuming that is correct your suggestion would not fix the issue.

I didn't know if it was permissible to add the include file in the block that follows line 119?
Or let mbedtls/config.h decide if MBEDTLS_DEBUG is set or not. I.e. deleting line 119?

Copy link
Member

@bagder bagder Oct 6, 2020

Choose a reason for hiding this comment

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

I think the #undef of the debug define probably should be removed. Or at least moved to the top of the file so that it is easier to find.

Copy link
Contributor Author

@fwh-dc fwh-dc Oct 6, 2020

Choose a reason for hiding this comment

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

I think the proper way to do it would be to include mbedtls/config.h . But I am not 100% certain that it does not introduce some regression. So I have moved the #undef to the top.

Copy link
Contributor

@emilengler emilengler Oct 6, 2020

Choose a reason for hiding this comment

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

I think undefing stuff we haven't defined is a bad idea and will create lots of confusion. If people invoke the compilation with extra CPP definitions, we shouldn't ignore them IMO.

Copy link
Contributor

@emilengler emilengler Oct 6, 2020

Choose a reason for hiding this comment

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

But again, I am not able to judge on this as I am not into the library 😊

Copy link
Contributor Author

@fwh-dc fwh-dc Oct 6, 2020

Choose a reason for hiding this comment

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

What do you think about the latest commit? It combines the previous functionality with your suggestion.

Copy link
Contributor

@emilengler emilengler Oct 6, 2020

Choose a reason for hiding this comment

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

Do you mean d06f3e1?
If so it's not perfect. Double slash comments are not allowed on the one hand, on the second hand I don't think we should leave unused Code commented out, especially when it has never been in use

bagder added a commit that referenced this issue Oct 7, 2020
bagder added a commit that referenced this issue Oct 7, 2020
@bagder bagder closed this in bc5455f Oct 7, 2020
@bagder
Copy link
Member

@bagder bagder commented Oct 7, 2020

Thanks! I squashed the commits and edited it slightly before I merged....

bagder added a commit that referenced this issue Oct 7, 2020
Spotted while working on #6045

Closes #6048
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

3 participants