Skip to content

storage: bm_storage: use errno instead of nrf_error#440

Merged
eivindj-nordic merged 3 commits into
nrfconnect:mainfrom
eivindj-nordic:error_code_alignment_storage
Nov 7, 2025
Merged

storage: bm_storage: use errno instead of nrf_error#440
eivindj-nordic merged 3 commits into
nrfconnect:mainfrom
eivindj-nordic:error_code_alignment_storage

Conversation

@eivindj-nordic
Copy link
Copy Markdown
Contributor

Use errnos for BM storage as it is not a BLE library or BLE subsystem.

@eivindj-nordic eivindj-nordic self-assigned this Oct 24, 2025
@eivindj-nordic eivindj-nordic requested review from a team as code owners October 24, 2025 10:08
@eivindj-nordic eivindj-nordic added this to the v1.0.0 milestone Oct 24, 2025
@github-actions github-actions Bot added changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. doc-required PR must not be merged without tech writer approval. labels Oct 24, 2025
@github-actions
Copy link
Copy Markdown

You can find the documentation preview for this PR here.

@eivindj-nordic eivindj-nordic force-pushed the error_code_alignment_storage branch 3 times, most recently from 4e7be0e to bcee21f Compare November 4, 2025 12:24
@eivindj-nordic eivindj-nordic requested review from a team and rghaddab as code owners November 4, 2025 12:24
@eivindj-nordic eivindj-nordic force-pushed the error_code_alignment_storage branch from bcee21f to 594ff03 Compare November 4, 2025 15:18
Copy link
Copy Markdown
Contributor

@anhmolt anhmolt left a comment

Choose a reason for hiding this comment

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

Could mention in the commit that
NRF_ERROR_INVALID_ADDR and NRF_ERROR_NULL are merged to -EFAULT, and
NRF_ERROR_INVALID_STATE and NRF_ERROR_FORBIDDEN are merged to -EPERM.
Also, is this something we want to do?

Comment thread lib/bluetooth/ble_adv/ble_adv_data.c
Comment thread samples/peripherals/storage/src/main.c
Comment thread subsys/storage/bm_storage/sd/bm_storage_sd.c Outdated
Comment thread tests/subsys/storage/bm_storage/src/unity_test.c Outdated
Comment thread include/bm/storage/bm_storage_backend.h
Comment thread include/bm/storage/bm_storage_backend.h
Comment thread subsys/fs/bm_zms/bm_zms.c Outdated
@eivindj-nordic eivindj-nordic force-pushed the error_code_alignment_storage branch from 594ff03 to dc0ef45 Compare November 5, 2025 08:28
@eivindj-nordic eivindj-nordic requested a review from a team as a code owner November 5, 2025 08:28
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is this file? Looks like it was generated by nrfutil? It does probably not belong here and will be overwritten on next SoftDevice export.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct, it should not be here. @nordicjm Should this be added to .gitignore or can it be rendered in the build folder or so instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@eivindj-nordic eivindj-nordic force-pushed the error_code_alignment_storage branch from dc0ef45 to 7783e7f Compare November 5, 2025 09:24

* Updated to use errno instead of nrf_errors.


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Extra newline

Comment thread include/bm/storage/bm_storage.h Outdated
* 0 on success.
* A negative errno otherwise.
*/
uint32_t result;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should be int

* @retval 0 on success.
* @retval -EFAULT If @p storage is @c NULL.
* @retval -EBUSY If the implementation-specific resource is busy.
* @retval -EIO If an implementation-specific internal error occurred.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would use EFAULT for generic failures. IO is more for read/write and such.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We use EFAULT for NULL checks.

Copy link
Copy Markdown
Contributor

@MirkoCovizzi MirkoCovizzi Nov 6, 2025

Choose a reason for hiding this comment

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

No, not in every library. It is not a standard. For user-space, the standard is -EINVAL for invalid parameters, including NULL pointers.

* @retval NRF_ERROR_BUSY If the implementation-specific resource is busy.
* @retval NRF_ERROR_INTERNAL If an implementation-specific internal error occurred.
* @retval 0 on success.
* @retval -EFAULT If @p storage is @c NULL.
Copy link
Copy Markdown
Contributor

@MirkoCovizzi MirkoCovizzi Nov 5, 2025

Choose a reason for hiding this comment

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

EINVAL? I prefer to use EINVAL here, but I know that we don't really have a standard for this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the standard in the repo.

Copy link
Copy Markdown
Contributor

@MirkoCovizzi MirkoCovizzi Nov 6, 2025

Choose a reason for hiding this comment

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

No, it is not a standard. Some libraries in this repo do it this way. But it's ok, it can stay like this, it's not a big deal.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll leave it as is, as it is like this in many other libraries. If we see the need we can align this treewide in a separate PR.

Fix compliance issue.

Signed-off-by: Eivind Jølsgard <eivind.jolsgard@nordicsemi.no>
@eivindj-nordic eivindj-nordic force-pushed the error_code_alignment_storage branch from 7783e7f to cd7a854 Compare November 5, 2025 14:57
Comment thread include/bm/storage/bm_storage_backend.h Outdated
* @retval NRF_ERROR_FORBIDDEN If the implementation-specific backend has not been initialized.
* @retval 0 on success.
* @retval -EPERM If the implementation-specific backend has not been initialized.
* @retval -EFAULT if src is not 32-bit aligned.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@p src

Use errnos for BM storage as it is not a BLE library or BLE subsystem.
Correct return values for BM ZMS.

Signed-off-by: Eivind Jølsgard <eivind.jolsgard@nordicsemi.no>
ring_buf_get() returns the size retrieved and will never be negative.

Signed-off-by: Eivind Jølsgard <eivind.jolsgard@nordicsemi.no>
@eivindj-nordic eivindj-nordic force-pushed the error_code_alignment_storage branch from cd7a854 to efb4e74 Compare November 6, 2025 09:27
@eivindj-nordic eivindj-nordic merged commit b5615ab into nrfconnect:main Nov 7, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. doc-required PR must not be merged without tech writer approval.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants