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

Consider naming structs so it is possible to forward declare them #1215

Closed
randombit opened this Issue Dec 18, 2017 · 6 comments

Comments

Projects
None yet
6 participants
@randombit

randombit commented Dec 18, 2017

Currently in ecp.h the mbedtls_ecp_keypair struct is defined as

typedef struct {
  // various values
} mbedtls_ecp_keypair;

The problem arises, I am currently working on a codebase that supports both OpenSSL and mbedtls. For the OpenSSL case I have something like typedef struct ec_key_st MY_ECC_KEY and then use MY_ECC_KEY* as parameters to various functions. But there is afaict no way to provide a similar forward declaration for mbedtls_ecp_keypair, because the struct has no name! Instead I must actually include the mbedtls header in order to get the typedef.

It would be nice if this could be fixed in a future release. I believe all that is required is to grep for "typedef struct {" and for each case insert the same name as the typedef. Then my code would not have to include mbedtls in the header, and I could use

typedef struct mbedtls_ecp_keypair MY_ECC_KEY.

(This already works for some structs, like mbedtls_x509_crt)

I think this would be useful outside my case as I imagine there are many applications that support mbedtls as well as another implementation, and being able to forward declare types allows making the implementation more opaque to the larger codebase.

@RonEld

This comment has been minimized.

Contributor

RonEld commented Dec 19, 2017

HI @randombit Thank you for your suggestion.
We would take this into consideration.

As an open source project, we welcome contributions from the community, as long as the contributor followed our coding standards and signed the CLA.

@ciarmcom ciarmcom added the mirrored label Dec 19, 2017

@ciarmcom

This comment has been minimized.

Member

ciarmcom commented Dec 19, 2017

ARM Internal Ref: IOTSSL-1951

@deleisha

This comment has been minimized.

deleisha commented Jan 4, 2018

Just did a grep on typedef struct and found that it is less than 80 places where the possible changes might takes place, excluding tests and program folder.
Let me know if you want a volunteer. I intend to change by reusing the typedef name as also the struct TAG, for example

typedef struct
{
    uint32_t P[MBEDTLS_BLOWFISH_ROUNDS + 2];    /*!<  Blowfish round keys    */
    uint32_t S[4][256];                 /*!<  key dependent S-boxes  */
}
mbedtls_blowfish_context;

to

typedef struct mbedtls_blowfish_context
{
    uint32_t P[MBEDTLS_BLOWFISH_ROUNDS + 2];    /*!<  Blowfish round keys    */
    uint32_t S[4][256];                 /*!<  key dependent S-boxes  */
}
mbedtls_blowfish_context;
@gelldur

This comment has been minimized.

Contributor

gelldur commented Jul 13, 2018

I have same problem now. What I found that there is ifdef for including #include "blowfish_alt.h" but such file doesn't exist?!

I will prepare PR

@RonEld

This comment has been minimized.

Contributor

RonEld commented Jul 15, 2018

@gelldur

What I found that there is ifdef for including #include "blowfish_alt.h" but such file doesn't exist?!

This is by design. If you can implement an alternative functionality of blowfish on your platform, such as HW acceleration, you should define blowfish_alt.h and the relevant structure.
For more information, you can read this article

gelldur added a commit to gelldur/mbedtls that referenced this issue Jul 24, 2018

Allow to forward declare of public structs ARMmbed#1215
Thanks to forward declare we can declare `struct` in our header file instead making #include
@sbutcher-arm

This comment has been minimized.

Collaborator

sbutcher-arm commented Sep 13, 2018

PR #1861 merged so this issue can now be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment