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

Check return of BlockDevice::init() in TDBStore. #11595

Merged
merged 1 commit into from
Oct 1, 2019

Conversation

SeppoTakalo
Copy link
Contributor

Description

Return value was ignored, and TDBStore:init() ended up in a
MBED_ERROR() phase after that.

TDBStore API was limited to allow returning of only two separate
errors, which may end up hiding the actual return value. Change
the documentation slightly to allow returning of original error
code from the underlying block device.

Fixes #11591

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[x] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@VeijoPesonen

Release Notes

TDBSTore::init() is now allowed to return error codes from underlying BlockeDevice.
Previously the API was limited to return only MBED_ERROR_READ_FAILED or MBED_ERROR_WRITE_FAILED which might have hidden the actual error code from the device.
Now application will get the original return value passed from BlockDevice.

Copy link
Contributor

@VeijoPesonen VeijoPesonen left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 30, 2019

Astyle shows one error , otherwise looks fine

Return value was ignored, and TDBStore:init() ended up in a
MBED_ERROR() phase after that.

TDBStore API was limited to allow returning of only two separate
errors, which may end up hiding the actual return value. Change
the documentation slightly to allow returning of original error
code from the underlying block device.

Fixes ARMmbed#11591
@SeppoTakalo
Copy link
Contributor Author

Astyle should be fixed now.

@ciarmcom
Copy link
Member

@SeppoTakalo, thank you for your changes.
@VeijoPesonen @ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

@mbed-ci
Copy link

mbed-ci commented Sep 30, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@pauluap
Copy link

pauluap commented Sep 30, 2019

In resolving a problem I had with this issue (#11591), there are two possible problems with this approach:

  1. There already is a fail label in the file, at line 502. I'm not up on my goto skills, but I am surprised that the file compiles with two identical labels in different places. I had thought that goto labels only has file scope, so I'm uncertain that the behavior will be restricted to function scope and calling goto fail would work in both TDBStore::set_start and TDBStore::init
  2. There are many functions in the public interface that do not check if the instance has been initialized (there are some that do, however), and calling those would lead to badness. In the ticket referenced above, I have made changes to those functions.

@VeijoPesonen
Copy link
Contributor

I'm not up on my goto skills, but I am surprised that the file compiles with two identical labels in different places. I had thought that goto labels only has file scope, so I'm uncertain that the behavior will be restricted to function scope and calling goto fail would work in both TDBStore::set_start and TDBStore::init

Chapter 6.1 Labeled statement - Programming Languages — C++:

The only use of an identifier label is as the target of a goto. The scope of a label is the function in which it appears. Labels shall not be redeclared within a function. A label can be used in a goto statement before its definition. Labels have their own name space and do not interfere with other identifiers.

@SeppoTakalo
Copy link
Contributor Author

Using API that returns failure after init() call might lead to badness, which is a failure on the application, not the API.

Protective programming would allow applications to misbehave without crashing, which leads to errors being ignored, because its "up and running" in the testing phase. So crashing is what I wan't.

@SeppoTakalo
Copy link
Contributor Author

@0xc0170 Ping, this is reviewed and already passed the tests.

Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

I wonder why reinterpret cast is needed but its all over the file for these handles

@0xc0170 0xc0170 merged commit f5b5989 into ARMmbed:master Oct 1, 2019
@SeppoTakalo SeppoTakalo deleted the IOTSTOR-944 branch October 1, 2019 12:49
@adbridge adbridge added release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 and removed release-version: 5.15.0-rc1 labels Oct 3, 2019
@0xc0170 0xc0170 added release-version: 5.15.0-rc1 and removed release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 labels Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Result of block device init is ignored in TDBStore
7 participants