Skip to content

littlefs: Fixed issue with cleanup in mount function on error #7851

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

Merged
merged 2 commits into from
Aug 23, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 17 additions & 12 deletions features/filesystem/littlefs/LittleFileSystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ int LittleFileSystem::mount(BlockDevice *bd)
_bd = bd;
int err = _bd->init();
if (err) {
_bd = NULL;
LFS_INFO("mount -> %d", err);
_mutex.unlock();
return err;
Expand Down Expand Up @@ -164,36 +165,40 @@ int LittleFileSystem::mount(BlockDevice *bd)
}

err = lfs_mount(&_lfs, &_config);
LFS_INFO("mount -> %d", lfs_toerror(err));
if (err) {
_bd = NULL;
LFS_INFO("mount -> %d", lfs_toerror(err));
_mutex.unlock();
return lfs_toerror(err);
}

_mutex.unlock();
return lfs_toerror(err);
LFS_INFO("mount -> %d", 0);
return 0;
}

int LittleFileSystem::unmount()
{
_mutex.lock();
LFS_INFO("unmount(%s)", "");
int res = 0;
if (_bd) {
int err = lfs_unmount(&_lfs);
if (err) {
LFS_INFO("unmount -> %d", lfs_toerror(err));
_mutex.unlock();
return lfs_toerror(err);
if (err && !res) {
Copy link

@deepikabhavnani deepikabhavnani Aug 23, 2018

Choose a reason for hiding this comment

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

Just a nit: !res check is not really needed here Too late :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I just kept it for consistency. That way if any code is added between the declaration of res and this line, it won't break anything.

The compiler will optimize it out anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

The compiler will optimize it out anyways.

Famous last words

res = lfs_toerror(err);
}

err = _bd->deinit();
if (err) {
LFS_INFO("unmount -> %d", err);
_mutex.unlock();
return err;
if (err && !res) {
res = err;
}

_bd = NULL;
}

LFS_INFO("unmount -> %d", 0);
LFS_INFO("unmount -> %d", res);
_mutex.unlock();
return 0;
return res;
}

int LittleFileSystem::format(BlockDevice *bd,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,199 @@
/* mbed Microcontroller Library
* Copyright (c) 2017 ARM Limited
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#include "mbed.h"
#include "greentea-client/test_env.h"
#include "unity.h"
#include "utest.h"
#include <stdlib.h>
#include <errno.h>

using namespace utest::v1;

// test configuration
#ifndef MBED_TEST_FILESYSTEM
#define MBED_TEST_FILESYSTEM LittleFileSystem
#endif

#ifndef MBED_TEST_FILESYSTEM_DECL
#define MBED_TEST_FILESYSTEM_DECL MBED_TEST_FILESYSTEM fs("fs")
#endif

#ifndef MBED_TEST_BLOCKDEVICE
#error [NOT_SUPPORTED] Non-volatile block device required
#endif

#ifndef MBED_TEST_BLOCKDEVICE_DECL
#define MBED_TEST_BLOCKDEVICE_DECL MBED_TEST_BLOCKDEVICE bd
#endif

#ifndef MBED_TEST_FILES
#define MBED_TEST_FILES 4
#endif

#ifndef MBED_TEST_DIRS
#define MBED_TEST_DIRS 4
#endif

#ifndef MBED_TEST_BUFFER
#define MBED_TEST_BUFFER 8192
#endif

#ifndef MBED_TEST_TIMEOUT
#define MBED_TEST_TIMEOUT 480
#endif


// declarations
#define STRINGIZE(x) STRINGIZE2(x)
#define STRINGIZE2(x) #x
#define INCLUDE(x) STRINGIZE(x.h)

#include INCLUDE(MBED_TEST_FILESYSTEM)
#include INCLUDE(MBED_TEST_BLOCKDEVICE)

MBED_TEST_FILESYSTEM_DECL;
MBED_TEST_BLOCKDEVICE_DECL;

Dir dir[MBED_TEST_DIRS];
File file[MBED_TEST_FILES];
DIR *dd[MBED_TEST_DIRS];
FILE *fd[MBED_TEST_FILES];
struct dirent ent;
struct dirent *ed;
size_t size;
uint8_t buffer[MBED_TEST_BUFFER];
uint8_t rbuffer[MBED_TEST_BUFFER];
uint8_t wbuffer[MBED_TEST_BUFFER];


// tests for integration level format operations

void test_format()
{
int res = bd.init();
TEST_ASSERT_EQUAL(0, res);

{
res = MBED_TEST_FILESYSTEM::format(&bd);
TEST_ASSERT_EQUAL(0, res);
}

res = bd.deinit();
TEST_ASSERT_EQUAL(0, res);
}

void test_mount()
{
int res = bd.init();
TEST_ASSERT_EQUAL(0, res);

{
res = fs.mount(&bd);
TEST_ASSERT_EQUAL(0, res);
res = fs.unmount();
TEST_ASSERT_EQUAL(0, res);
}

res = bd.deinit();
TEST_ASSERT_EQUAL(0, res);
}

void test_bad_mount()
{
int res = bd.init();
TEST_ASSERT_EQUAL(0, res);

{
res = bd.erase(0, 2*bd.get_erase_size());
TEST_ASSERT_EQUAL(0, res);
memset(buffer, 0, bd.get_program_size());
for (int i = 0; i < 2*bd.get_erase_size(); i += bd.get_program_size()) {
res = bd.program(buffer, i, bd.get_program_size());
TEST_ASSERT_EQUAL(0, res);
}
}

{
res = fs.mount(&bd);
TEST_ASSERT_NOT_EQUAL(0, res);
}

res = bd.deinit();
TEST_ASSERT_EQUAL(0, res);
}

void test_bad_mount_then_reformat()
{
int res = bd.init();
TEST_ASSERT_EQUAL(0, res);

{
res = fs.mount(&bd);
TEST_ASSERT_NOT_EQUAL(0, res);

res = fs.reformat(&bd);
TEST_ASSERT_EQUAL(0, res);

res = fs.unmount();
TEST_ASSERT_EQUAL(0, res);
}

res = bd.deinit();
TEST_ASSERT_EQUAL(0, res);
}

void test_good_mount_then_reformat()
{
int res = bd.init();
TEST_ASSERT_EQUAL(0, res);

{
res = fs.mount(&bd);
TEST_ASSERT_EQUAL(0, res);

res = fs.reformat(&bd);
TEST_ASSERT_EQUAL(0, res);

res = fs.unmount();
TEST_ASSERT_EQUAL(0, res);
}

res = bd.deinit();
TEST_ASSERT_EQUAL(0, res);
}


// test setup
utest::v1::status_t test_setup(const size_t number_of_cases)
{
GREENTEA_SETUP(MBED_TEST_TIMEOUT, "default_auto");
return verbose_test_setup_handler(number_of_cases);
}

Case cases[] = {
Case("Test format", test_format),
Case("Test mount", test_mount),
Case("Test bad mount", test_bad_mount),
Case("Test bad mount than reformat", test_bad_mount_then_reformat),
Case("Test good mount than reformat", test_good_mount_then_reformat),
};

Specification specification(test_setup, cases);

int main()
{
return !Harness::run(specification);
}