Skip to content

sys/luid: Added postcondition to luid_get()#12604

Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom
maribu:luid-doc
Jun 16, 2020
Merged

sys/luid: Added postcondition to luid_get()#12604
miri64 merged 1 commit intoRIOT-OS:masterfrom
maribu:luid-doc

Conversation

@maribu
Copy link
Member

@maribu maribu commented Oct 29, 2019

Contribution description

Stated the obvious in the doc: Altering the LUID returned by luid_get() breaks the guarantee that it is locally unique, as it than could collide with other LUIDs returned by luid_get().

Testing procedure

Read the new documentation and verify its correctness.

Issues/PRs references

Related to #12600, #12592. Incorrect users of this API have been identified by @benpicco in #12592

Stated the obvious in the doc: Altering the LUID returned by luid_get() breaks
the guarantee that it is locally unique, as it than could collide with other
LUIDs returned by luid_get().
@maribu maribu added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: doc Area: Documentation labels Oct 29, 2019
@maribu maribu added the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Oct 29, 2019
@benpicco
Copy link
Contributor

benpicco commented Oct 29, 2019

If you grep for luid_get() you will find that all users (except for the one in random.c) will violate the API now 🤣

@maribu
Copy link
Member Author

maribu commented Oct 29, 2019

I truly believe that the added doc only clarifies what is already the case. So from my point of view this only makes it easier to notice that they all are violating the API.

But I assume this could be controversial in the context it now explicitly says all users but one of the API violate the API; so maybe ACK > 1 is needed.

@maribu maribu added Discussion: needs consensus Marks a discussion that needs a (not necessarily moderated) consensus Process: needs >1 ACK Integration Process: This PR requires more than one ACK labels Oct 29, 2019
@miri64
Copy link
Member

miri64 commented Oct 29, 2019

This precondition makes it basically unusable for the originally intended use-case. ^^

@benpicco
Copy link
Contributor

Adding luid_get_eui64() and luid_get_eui48() and converting the existing drivers to that is probably the only sane solution here.

Then this change should be uncontroversial.

@maribu
Copy link
Member Author

maribu commented Oct 29, 2019

I think it is useless for the apprently most common use case. The new doc will only point that clearly out.

@stale
Copy link

stale bot commented May 1, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label May 1, 2020
@benpicco
Copy link
Contributor

benpicco commented May 2, 2020

The description is still true though - and we have luid_get_eui64()/luid_get_eui48() now, so there is an alternative to the wrong use.

@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label May 2, 2020
@benpicco benpicco added the CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs label May 2, 2020
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 25, 2020
@fjmolinas
Copy link
Contributor

Everything is green here, is there something blocking this still @miri64 @benpicco?

@miri64
Copy link
Member

miri64 commented Jun 16, 2020

Nope, just fine IMHO. Let's just rerun Travis, to be safe.

@miri64 miri64 merged commit 1f2c3f7 into RIOT-OS:master Jun 16, 2020
@maribu maribu deleted the luid-doc branch June 16, 2020 13:51
@maribu
Copy link
Member Author

maribu commented Jun 16, 2020

Thanks for the reviews!

@miri64 miri64 added this to the Release 2020.07 milestone Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: doc Area: Documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs Discussion: needs consensus Marks a discussion that needs a (not necessarily moderated) consensus Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Process: needs >1 ACK Integration Process: This PR requires more than one ACK Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants