Skip to content

Add a new header file for private functions #395

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jun 3, 2018
Merged

Add a new header file for private functions #395

merged 5 commits into from
Jun 3, 2018

Conversation

@sjaeckel
Copy link
Member

@sjaeckel sjaeckel commented May 31, 2018 •
edited
Loading

This adds tomcrypt_private.h with all the declarations that were protected by LTC_SOURCE before.

Additionally this makes blake2[sb]_init() private.
I'd accompany that change by marking both functions as deprecated in the upcoming 1.18.2 release.
[Edit]: I just realized we can't simply deprecate them w/o having our source complain as well on compilation... I'll have to think about it a bit more...

@sjaeckel sjaeckel requested review from karel-m and ksherlock May 31, 2018 12:41
@sjaeckel sjaeckel force-pushed the feature/private_header branch from 65cc288 to 4091768 Compare May 31, 2018 12:45
karel-m
karel-m reviewed Jun 1, 2018
View reviewed changes
src/headers/tomcrypt_private.h Outdated

/* tomcrypt_math.h */

#if !defined(DESC_DEF_ONLY) && defined(LTC_SOURCE)
Copy link
Member

@karel-m karel-m Jun 1, 2018

Choose a reason for hiding this comment

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

drop && defined(LTC_SOURCE)

Copy link
Member Author

@sjaeckel sjaeckel Jun 2, 2018

Choose a reason for hiding this comment

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

done

karel-m
karel-m reviewed Jun 1, 2018
View reviewed changes
src/headers/tomcrypt_private.h Outdated
*/

typedef struct Oid {
const unsigned long *OID;
Copy link
Member

@karel-m karel-m Jun 1, 2018

Choose a reason for hiding this comment

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

unsigned long OID[16];

Copy link
Member Author

@sjaeckel sjaeckel Jun 2, 2018

Choose a reason for hiding this comment

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

oops, that one slipped in from my pkcs8 refactor... :)

karel-m
karel-m reviewed Jun 1, 2018
View reviewed changes
makefile.unix Outdated
-@rm -f test.exe constants.exe sizes.exe tv_gen.exe hashsum.exe ltcrypt.exe small.exe timing.exe

#Install the library + headers
install: $(LIBMAIN_S) $(HEADERS)
Copy link
Member

@karel-m karel-m Jun 1, 2018

Choose a reason for hiding this comment

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

maybe install: $(LIBMAIN_S) $(HEADERS_PUB)

Copy link
Member Author

@sjaeckel sjaeckel Jun 2, 2018

Choose a reason for hiding this comment

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

I removed it entirely, as it's already an indirect dependency of LIBMAIN_S

@karel-m
Copy link
Member

karel-m commented Jun 1, 2018

consider also changing makefile_include.mk like this:

- install -p -m 644 $(HEADERS) $(DESTDIR)$(INCPATH)
+ install -p -m 644 $(HEADERS_PUB) $(DESTDIR)$(INCPATH)

@karel-m
Copy link
Member

karel-m commented Jun 1, 2018

One more LTC_SOURCE to eliminate is in doc/Doxyfile

PREDEFINED             = LTC_SOURCE \
                         LTM_DESC \
                         GMP_DESC \
                         TFM_DESC

@sjaeckel sjaeckel force-pushed the feature/private_header branch from 4091768 to 7e65c6b Compare June 2, 2018 11:12
@sjaeckel
Copy link
Member Author

sjaeckel commented Jun 2, 2018 •
edited
Loading

I'd accompany that change by marking both functions as deprecated in the upcoming 1.18.2 release.
[Edit]: I just realized we can't simply deprecate them w/o having our source complain as well on compilation... I'll have to think about it a bit more...

regarding the deprecation I think we shouldn't do that in 1.18.2 but only in develop (as already done for the math initializers). To prevent warnings in our internal code I'd leave the blake2[sb]_init() API as deprecated public and make it call the newly added _blake2[sb]_init() which contain the implementation. Okay?

@ksherlock
Copy link
Collaborator

ksherlock commented Jun 2, 2018

I'm probably the only one, but I use blake2s_init() function from user code to generate hashes with non-standard digest sizes (128, 160, 224, and 256 are the most common but it can generate digests of 8-512 bits. There's also a configurable key parameter).

@sjaeckel
Copy link
Member Author

sjaeckel commented Jun 2, 2018

Oh okay, then we'll leave them in the public API!

@sjaeckel sjaeckel force-pushed the feature/private_header branch from 7e65c6b to e1caead Compare June 3, 2018 11:29
@sjaeckel sjaeckel force-pushed the feature/private_header branch from e1caead to c725be2 Compare June 3, 2018 13:07
karel-m
karel-m approved these changes Jun 3, 2018
View reviewed changes
Copy link
Member

@karel-m karel-m left a comment

Choose a reason for hiding this comment

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

now looks good

@sjaeckel sjaeckel merged commit 55fbe25 into develop Jun 3, 2018
@sjaeckel sjaeckel deleted the feature/private_header branch June 3, 2018 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@karel-m karel-m karel-m approved these changes

@ksherlock ksherlock Awaiting requested review from ksherlock

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sjaeckel @karel-m @ksherlock