-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Remove minor build warnings #11392
Remove minor build warnings #11392
Conversation
@hugueskamba, thank you for your changes. |
@@ -1203,6 +1203,8 @@ void sn_coap_protocol_linked_list_duplication_info_remove(struct coap_s *handle, | |||
(void)msg_id; | |||
#endif //SN_COAP_DUPLICATION_MAX_MSGS_COUNT | |||
} | |||
|
|||
#if SN_COAP_DUPLICATION_MAX_MSGS_COUNT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All other references to sn_coap_protocol_duplication_info_free are already flagged by SN_COAP_DUPLICATION_MAX_MSGS_COUNT and therefore this is good to go.
CC: @anttiylitokola
@@ -869,7 +869,7 @@ attest_create_token(struct useful_buf_c *challenge, | |||
token_err = attest_token_start(&attest_token_ctx, | |||
option_flags, /* option_flags */ | |||
key_select, /* key_select */ | |||
COSE_ALGORITHM_ES256, /* alg_select */ | |||
alg_select, /* alg_select */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ARMmbed/mbed-os-crypto This one also should be fixed in tfm repository?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code in the trusted firmware repo is different than the version in Mbed OS.
The variable alg_select
has been added to the Mbed OS version along with
/* Map the key select to an algorithm. Maybe someday we'll support something
* other than ES256
*/
switch (key_select) {
default:
alg_select = COSE_ALGORITHM_ES256;
}
Therefore no fix is necessary upstream.
Is the file usually synchronized with the upstream version? If so, should the Mbed OS version of the file (well the function at least) be changed instead to mirror its upstream counterpart?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, agreed that no change upstream is necessary.
Yes, we should take the upstream version when we replace our attestation service with the TF-M one.
In the mean time, I'd suggest removing the switch to fix the warning, as that keeps the code better aligned with upstream TF-M.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for fixing warnings |
As the variable underlying type size is different depending on the toolchain used
afa9698
to
8c22bbb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting for crypto and mesh team to review
a9abdac
to
9715587
Compare
@@ -869,7 +869,7 @@ attest_create_token(struct useful_buf_c *challenge, | |||
token_err = attest_token_start(&attest_token_ctx, | |||
option_flags, /* option_flags */ | |||
key_select, /* key_select */ | |||
COSE_ALGORITHM_ES256, /* alg_select */ | |||
alg_select, /* alg_select */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All changes approved
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
Description
The various warnings removed were observed when building
mbed-os-example-blinky
for theK64F
target with theGCC_ARM
toolchain.Pull request type
Reviewers
@kjbracey-arm @evedon
Release Notes