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.

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

Changing CiperSuiteId from single to multiple resource #1404

Closed
wants to merge 14 commits into from
Closed

Changing CiperSuiteId from single to multiple resource #1404

wants to merge 14 commits into from

Conversation

Warmek
Copy link
Contributor

@Warmek Warmek commented Feb 27, 2023 •
edited
Loading

Changing CipherSuiteId field in BootstrapConfig.java from single resource to multiple resource as to match OMA specification.

It aims to implement #1402

@Warmek Warmek mentioned this pull request Feb 27, 2023
Closed
sbernard31
sbernard31 reviewed Feb 27, 2023
View reviewed changes
leshan-server-core/src/main/java/org/eclipse/leshan/server/bootstrap/BootstrapConfig.java Outdated Show resolved Hide resolved
leshan-server-core/src/main/java/org/eclipse/leshan/server/bootstrap/BootstrapConfig.java Show resolved Hide resolved
leshan-server-core/src/main/java/org/eclipse/leshan/server/bootstrap/BootstrapConfig.java Outdated Show resolved Hide resolved
leshan-server-core/src/main/java/org/eclipse/leshan/server/bootstrap/BootstrapConfig.java Outdated Show resolved Hide resolved
leshan-server-core/src/main/java/org/eclipse/leshan/server/bootstrap/BootstrapConfig.java Outdated Show resolved Hide resolved
leshan-core/src/main/java/org/eclipse/leshan/core/node/LwM2mSingleResource.java Outdated Show resolved Hide resolved
leshan-core/src/main/java/org/eclipse/leshan/core/node/LwM2mSingleResource.java Outdated Show resolved Hide resolved
@sbernard31
Copy link
Contributor

About the title of the PR, better to find a title explaining the content of the PR instead of name of your branch.

About the description, this seems to not really describe the content of the PR (by the way it point to an issue which doesn't exist)

About your git config, you still don't change your it like asked at (#1326 (comment), #1377 (comment))

Do not hesitate to ask for help (to me or maybe @JaroslawLegierski), if you face any issue.

@sbernard31
Copy link
Contributor

I looked at you last commit, still some (new?) unnecessary comment format changes.
(It should not have changes in LwM2mSingleResource class as finally you didn't add anything to it)

@sbernard31
Copy link
Contributor

This moves forward 🙂 but still missing some point :

  • ❌ About the title of the PR, better to find a title explaining the content of the PR instead of name of your branch.
  • ✔️ About the description ...
  • ✔️ About your git config ...
  • ❌ missing javadoc on constructor (see review above)

Like I said at #1402 (comment) , maybe javadoc could be a bit improved.

@Warmek Warmek changed the title Opl/cipher suite Changing CiperSuiteId from single to multiple resources Feb 28, 2023
@Warmek Warmek changed the title Changing CiperSuiteId from single to multiple resources Changing CiperSuiteId from single to multiple resource Feb 28, 2023
sbernard31
sbernard31 reviewed Feb 28, 2023
View reviewed changes
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.

I added some comments about javadoc.

You should also review your PR, I guess you will find some unexpected code changes. 🙂

leshan-server-core/src/main/java/org/eclipse/leshan/server/bootstrap/BootstrapConfig.java Outdated Show resolved Hide resolved
leshan-server-core/src/main/java/org/eclipse/leshan/server/bootstrap/BootstrapConfig.java Outdated Show resolved Hide resolved
leshan-server-core/src/main/java/org/eclipse/leshan/server/bootstrap/BootstrapConfig.java Outdated Show resolved Hide resolved
sbernard31
sbernard31 reviewed Feb 28, 2023
View reviewed changes
leshan-server-core/src/main/java/org/eclipse/leshan/server/bootstrap/BootstrapConfig.java Outdated Show resolved Hide resolved
leshan-server-core/src/main/java/org/eclipse/leshan/server/bootstrap/BootstrapConfig.java Outdated
Comment on lines 484 to 486
String binaryString = Long.toBinaryString(valueFromSecurityObject.longValue());
this.firstByte = (byte) Integer.parseInt(binaryString.substring(0, 8), 2);
this.secondByte = (byte) Integer.parseInt(binaryString.substring(9, 17), 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Using String is maybe not the best idea.

Should be better to play with binary operator like :

  • shift operator : >>

You can create unit test to check your code.

We should also check that valueFromSecurityObject is a 16-bit unsigned integer.

leshan-server-core/src/main/java/org/eclipse/leshan/server/bootstrap/BootstrapConfig.java Outdated
* @return Integer number concatenated from 2 bytes.
*/
public ULong getValueForSecurityObject() {
return ULong.valueOf(Byte.toUnsignedInt(firstByte) * 256 + Byte.toUnsignedInt(secondByte));
Copy link
Contributor

Choose a reason for hiding this comment

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

Using floating point number and multiplication are maybe not best idea.

Should be better to play with binary operator like :

  • bitMask operator : &
  • binary notation : 0b00000
  • shift operator : <<

You can create unit test to check your code.

@sbernard31
Copy link
Contributor

We should also check that valueFromSecurityObject is a 16-bit unsigned integer.

You didn't work on it ? is it on purpose ?

You can create unit test to check your code.

Do you plan to create junit test about it ? (mainly to test conversion and bad inputs)

Tell me when you reviewed your own code and consider I need to review it again. 🙂

@github-actions
Copy link

github-actions bot commented Mar 2, 2023

ℹ️ Some tips :

If documentation or those automatic comments are not clear enough, please create a new issue to discus about how to enhance it.

@github-actions
Copy link

github-actions bot commented Mar 2, 2023

❌ Java Import are not sorted ! (more details)

Ensure your code build locally using:

mvn clean install

Or just validate java import with :

mvn impsort:check

You can sort Java import with :

mvn impsort:sort

See also How configure your IDE.

@sbernard31
Copy link
Contributor

@Warmek ignore the sortimp issue, we have same problem on this PR : #1398 (comment)

I don't get it locally, I don't understand the problem for now 😬

@sbernard31
Copy link
Contributor

Let me know when you review your code and you satisfy about it and so it is ready for a review.

You could do that by posting a comment or requesting a review.

Warmek
Warmek commented Mar 3, 2023
View reviewed changes
Copy link
Contributor Author

@Warmek Warmek left a comment

Choose a reason for hiding this comment

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

@sbernard31 I reviewed changes and there seems to be no issues. Let me know what you think about it

@sbernard31 sbernard31 mentioned this pull request Mar 3, 2023
Merged
@sbernard31 sbernard31 mentioned this pull request Mar 3, 2023
Closed
@sbernard31
Copy link
Contributor

@Warmek I created a PR #1408 based on yours.

Could you review it ? 🙏 If wanted you can review commit by commit this will be easier to read.

@sbernard31
Copy link
Contributor

This work is included in #1408 which is now integrated in master.

@Warmek thx for your contribution.

@sbernard31 sbernard31 closed this Mar 3, 2023
@sbernard31 sbernard31 mentioned this pull request Mar 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbernard31 sbernard31 sbernard31 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Warmek @sbernard31