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

PSK identity related fixes #320

Closed
wants to merge 4 commits into from
Closed

PSK identity related fixes #320

wants to merge 4 commits into from

Conversation

scop
Copy link
Contributor

@scop scop commented Apr 12, 2017

No description provided.

Copy link
Contributor

@sbernard31 sbernard31 left a comment

Choose a reason for hiding this comment

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

Except details below, It sounds good to me.

if (info.getIdentity() != null) {
// populate the secondary index (security info by PSK id)
String oldEndpoint;
if ((oldEndpoint = j.hget(PSKID_SEC, info.getIdentity())) != null
Copy link
Contributor

Choose a reason for hiding this comment

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

In the Leshan code base, we generally don't initialize variable in if condition.
Could you please, move the initialization out of the if ?

String oldEndpoint =  j.hget(PSKID_SEC, info.getIdentity();
if (oldEndpoint != null ...

byte[] previousData = j.getSet((SEC_EP + info.getEndpoint()).getBytes(), data);
SecurityInfo previous = previousData == null ? null : deserialize(previousData);
String previousIdentity;
if (previous != null && (previousIdentity = previous.getIdentity()) != null
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing about variable initialization here.
If you really want to avoid to call several time previous.getIdentity() several time, you will probably need to create 2 if blocks. Personally I'm ok if you choose to not put previous.getIdentity() in a variable.

@@ -98,6 +98,11 @@ public SecurityInfo add(SecurityInfo info) throws NonUniqueSecurityInfoException
}

SecurityInfo previous = securityByEp.put(info.getEndpoint(), info);
String previousIdentity;
if (previous != null && (previousIdentity = previous.getIdentity()) != null
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment than above.

scop added 4 commits April 13, 2017 13:59
Signed-off-by: Ville Skyttä <ville.skytta@iki.fi>
Signed-off-by: Ville Skyttä <ville.skytta@iki.fi>
Signed-off-by: Ville Skyttä <ville.skytta@iki.fi>
Signed-off-by: Ville Skyttä <ville.skytta@iki.fi>
@scop
Copy link
Contributor Author

scop commented Apr 13, 2017

Comments addressed, rebased.

@sbernard31
Copy link
Contributor

Integrated in master (commit a907eda, 00db6ec, 349a292, ac0a48a)

I also removed securityInfo for GOOD_ENDPOINT in Security integration tests (d96abbb)

@sbernard31 sbernard31 closed this Apr 25, 2017
@scop scop deleted the psk-id-fixes branch April 25, 2017 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants