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 to forward declare of mbedtls_blowfish_context #1215 #1861

Conversation

gelldur
Copy link
Contributor

@gelldur gelldur commented Jul 13, 2018

Status

READY

Requires Backporting

NO

Migrations

NO

@gelldur
Copy link
Contributor Author

gelldur commented Jul 13, 2018

For issue: #1215

@gelldur
Copy link
Contributor Author

gelldur commented Jul 13, 2018

Probably I should extend this PR for other such structs.

@RonEld RonEld added enhancement CLA valid component-crypto Crypto primitives and low-level interfaces needs-review Every commit must be reviewed by at least two team members, labels Jul 15, 2018
@RonEld RonEld requested a review from yanesca July 15, 2018 07:25
@yanesca
Copy link
Contributor

yanesca commented Jul 19, 2018

@gelldur First of all thank you for your contribution! I agree, it would be best to extend other such structs too, because the issue (#1215) mentioned this as a general problem and blowfish was just an example.

@gelldur
Copy link
Contributor Author

gelldur commented Jul 20, 2018

@yanesca I added (i think) all needed forward declares

@yanesca
Copy link
Contributor

yanesca commented Jul 20, 2018

@gelldur Thanks for extending the fix! Could you please extend it a little bit further? I was thinking about these structures:

mbedtls_x509_crt_profile                                                        
mbedtls_havege_state                                                            
mbedtls_oid_descriptor_t                                                        
mbedtls_aes_xts_context                                                         
mbedtls_mpi                                                                     
mbedtls_pk_rsassa_pss_options                                                   
mbedtls_pk_debug_item                                                           
mbedtls_entropy_source_state                                                    
mbedtls_net_context

@yanesca
Copy link
Contributor

yanesca commented Jul 20, 2018

(There are some other structs in _internal headers, but those shouldn't be used by the application anyway and therefore it is safe to leave them as they are. Preventing the forward declaration even has a function in this case as it further discourages the use of the internal headers.)

@gelldur
Copy link
Contributor Author

gelldur commented Jul 23, 2018

@yanesca I updated. I'm not sure about your last comment. Did I add to many forward declare or it is ok ?

@yanesca
Copy link
Contributor

yanesca commented Jul 23, 2018

@gelldur Thank you! You didn't add too many, I was just justifying why I didn't ask you to add even more.

@yanesca
Copy link
Contributor

yanesca commented Jul 23, 2018

@gelldur Could you please rebase to have a single commit with all the changes and explain in the commit message why we are doing these changes?

Thanks to forward declare we can declare `struct` in our header file instead making #include
@gelldur gelldur force-pushed the allow_forward_declare_of_mbedtls_blowfish_context branch from 73619e1 to 428cc52 Compare July 24, 2018 08:03
@gelldur
Copy link
Contributor Author

gelldur commented Jul 24, 2018

@yanesca done

Copy link
Contributor

@yanesca yanesca left a comment

Choose a reason for hiding this comment

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

Thank you! It looks good to me.

@RonEld
Copy link
Contributor

RonEld commented Jul 24, 2018

Was it tested on other platforms, such as Windows?

Copy link
Contributor

@RonEld RonEld left a comment

Choose a reason for hiding this comment

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

I am approving this, however I would like to know how other compilers react to this change

@RonEld RonEld removed the needs-review Every commit must be reviewed by at least two team members, label Jul 24, 2018
@simonbutcher
Copy link
Contributor

Whilst reviewing this, I was wondering if we need some style guidelines or similar to give struct names a different name from the typedef's (eg. the idiom of typedef struct _mystruct {...} mystruct;), however, I found we already appear to reuse the same name, as you can see in
asn1.h.

As such, I'm approving the change, but I see no need to backport.

@simonbutcher
Copy link
Contributor

As an observation, this names all unnamed struct's apart from pk_internal.h and cipher_internal.h, which are internal and don't need this change.

@simonbutcher simonbutcher added the approved Design and code approved - may be waiting for CI or backports label Jul 31, 2018
@Patater Patater merged commit 428cc52 into Mbed-TLS:development Aug 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-crypto Crypto primitives and low-level interfaces enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants