Add the StorageLite feature #6915

davidsaada · 2018-05-15T15:42:59Z

Description

StorageLite is a storage solution, providing a key value store like API of set/get data or a reduced POSIX API of open-write-close and open-read-close.

Design & documentation by @offirko
General and file system tests by @theamirocohen

Handbook documentation PR

This PR depends on the following open PRs:
#6757 - Buffered block device
#6559 - Flash simulator block device
#6480 - NVStore allocate_key API
#6642 - Device key

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[x] Feature
[ ] Breaking change

davidsaada · 2018-05-15T15:43:30Z

@dannybenor @geky

0xc0170 · 2018-05-15T15:51:40Z

Let's make sure those dependencies are moving forward

geky

The implementation is looking good. I'm already impressed, I thought the timeline was too short for creating a new filesystem.

However, I have a problem with how this is being integrated.

We don't need to introduce a new API

We have an established storage API (POSIX) that has already been adopted by users. We should not change directions without a good reason.

If we must introduce a new storage API, the interface should be available for all storage options that support it. There’s no point in having multiple storage options if the users can’t transition between the storage options.
StorageLite should not be tightly coupled to security

There is no technical reason StorageLite and security must be coupled. If it is an API problem, then we should fix our API so we can effectively add storage features without limiting users.

How can we move forward?

Integrate StorageLite into the POSIX API
Introduce POSIX secure layer

If POSIX API is proven to be inefficient/unworkable:

Create KVStore API
Integrate StorageLite into KVStore API
Integrate LittleFS and FAT into KVStore API (POSIX adapter?)
Introduce KVStore secure layer

These two options satisfy the above issues. I'm open to alternatives, but until the above issues are solved, merging this PR will be more disruptive than beneficial.

geky · 2018-05-15T16:07:10Z

features/device_key/source/DeviceKey.h~HEAD

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#ifndef MBED_DEVICEKEY_H


I think this file got added by accident? (the ~HEAD part)

@geky Your view is clear and you presented it several times.
Review of this PR should be done in terms of code quality and not feature definitions. There are good reasons to keep two different APIs for storage but this is not the place to discuss this.
Will appreciate your help on code quality review

I hope you will find the review itself is constructive.

However this is the correct place to note why a PR should not be merged, otherwise the PR will be merged.

The Head file is part of the dependent DeviceKey PR and should be handled there. @yossi2le are you aware of this?

@davidsaada The DeviceKey PR looks fine, I have no idea why this file is showing on this PR.

Maybe it was accidentally added while rebasing. Will check.

No rush, you'll need to rebase when dependencies go in anyways

geky · 2018-05-15T16:16:41Z

features/filesystem/storagelite/source/StorageLiteFS.cpp

+    : FileSystem(name), _stlite(stlite), _flags(flags), _bd(0)
+{
+    MBED_ASSERT(stlite);
+    mount(_bd);


I had to remove this mount call because the _bd pointer was NULL and the init assert would fail.

geky · 2018-05-15T16:21:38Z

features/filesystem/storagelite/source/StorageLiteFS.cpp

+            int ret = _stlite->file_exists((const uint8_t *) path, strlen(path) + 1);
+            // Fail if file exists
+            if (ret == STORAGELITE_SUCCESS) {
+                return EEXIST;


Should this be negative?

geky · 2018-05-15T16:22:25Z

features/filesystem/storagelite/source/StorageLiteFS.cpp

+    }
+
+    // We don't support opening for read-write, nor for appending
+    if ((flags & O_RDWR) || (flags & O_APPEND)) {


Yay! Glad to see full flag support 👍

geky · 2018-05-15T16:24:34Z

features/filesystem/storagelite/source/StorageLiteFS.cpp

+int StorageLiteFS::file_open(mbed::fs_file_t *file, const char *path, int flags)
+{
+    file_handle_data_t *handle = new file_handle_data_t;
+    if (!handle) {


This won't come back as NULL, it will assert. I believe you need nothrow:

file_handle_data_t *handle = new (std::nothrow) file_handle_data_t;

geky · 2018-05-15T19:36:19Z

features/filesystem/storagelite/TESTS/storagelite/fs_tests/main.cpp

+    Case("StorageLiteFS_fread_with_fopen_w_mode", StorageLiteFS_fread_with_fopen_w_mode),
+    Case("StorageLiteFS_fread_closed_file", StorageLiteFS_fread_closed_file),
+    Case("StorageLiteFS_fread_twice_from_file", StorageLiteFS_fread_twice_from_file),
+    Case("StorageLiteFS_fread_twice_separated_with_fclose", StorageLiteFS_fread_twice_separated_with_fclose),


Good amount of tests! We need these on all filesystems!

geky · 2018-05-15T19:37:40Z

features/filesystem/storagelite/TESTS/storagelite/general_tests/main.cpp

+static const size_t bd_read_size           = 1;
+
+StorageLite *stlite = NULL;
+HeapBlockDevice bd(bd_size, bd_read_size, bd_prog_size, bd_erase_size);


We also have an MBED_TEST_SIM_BLOCKDEVICE which I think is useful here. Otherwise the CI will try to run this on platforms with very little RAM:
https://github.com/ARMmbed/mbed-os/blob/master/features/filesystem/littlefs/TESTS/filesystem_recovery/wear_leveling/main.cpp#L48-L58

Not sure whether you saw the beginning of this file, failing the support of this test if MBED_TEST_BLOCKDEVICE is not present. This should prevent the execution of this test on low memory boards.

Just to add: The test currently doesn't use the BD exported by MBED_TEST_BLOCKDEVICE. Reason is that I wanted to have this white box test on different types of BDs (in term of sizes). I'll try adding this BD to the list of tested BDs.

geky · 2018-05-15T19:38:56Z

features/filesystem/storagelite/TESTS/storagelite/whitebox/main.cpp

+static const char *file3_val1 = "Data value of file 3 is the following";
+static const char *file4_name = "This is the name of file4";
+static const char *file4_val1 = "Is this the value of file 4?";
+static const char *file4_val2 = "What the hell is the value of file 4, god damn it!";


General tip, even though this is a test - this is blowing static RAM probably unnecessarily - 4 words of RAM for 4 non-constant pointers.

Suggest static const char * const file4_name or static const char file4_name[].

The presence of the pointer const qualifier tends to fool one into thinking that the object is const.

It's in a test and probably optimized out anyways,

Indeed, but it's a common pattern I see, so I like to raise awareness.

And you're right, as these are static the compiler could well optimise the pointer storage away - if it's analysing enough of the source file at a time to be certain they're not modified.

Good catch @kjbracey-arm. Adding that extra const actually reduced code size, at least in the ARM GCC compiler.

geky · 2018-05-15T19:40:57Z

features/filesystem/storagelite/TESTS/storagelite/whitebox/main.cpp

@@ -0,0 +1,670 @@
+/*


Out of curiousity what does "whitebox" mean?

White box is a testing term meaning that the test is aware of the internal coding logic (opposite from black box). Here the test code is aware of edge cases (BD and erase sizes for example) and different code paths.

Ah! That makes perfect sense, thanks

geky · 2018-05-15T19:48:08Z

features/filesystem/storagelite/TESTS/storagelite/whitebox/main.cpp

+    Case("StorageLite: White box test",     white_box_test,    greentea_failure_handler),
+    Case("StorageLite: White box FS test",  white_box_fs_test, greentea_failure_handler),
+    Case("StorageLite: Multiple set test",  multi_set_test,    greentea_failure_handler),
+    Case("StorageLite: Error inject test",  error_inject_test, greentea_failure_handler),


This is a great number of tests, but I think we're missing a couple important ones:

Simulated power-resilience testing: example

Greentea-driven power-resilience testing: example

Simulated wear-level testing: example

Note: these depend on some common files:
https://github.com/ARMmbed/mbed-os/tree/master/features/filesystem/littlefs/TESTS/COMMON

These already exist on top of the filesystem API, so it shouldn't be too difficult to adopt them here. Until then it would be a risk to advertise power resilience.

Right. Adding reset and wear level tests now.

deepikabhavnani · 2018-05-15T22:05:21Z

features/filesystem/storagelite/source/StorageLiteFS.cpp

+
+int StorageLiteFS::mount(BlockDevice *bd)
+{
+    _bd = bd;


Error handling/checking of input parameters must be done, what if bd is NULL?

We don't check NULL pointers elsewhere. Worst case, code will hard fault, and the NULL dereference can easily be found with a debugger.

Yup, and it’s not good. Recently the code base I am touching I try to add where ever I can, hence pointed out. Can be found with debugger but not good for end product

Interesting, I think we disagree here. Should we also be asserting on misaligned pointers?

Error handling does not mean only asserting, but to actually avoid sudden crash and respond with proper errror message/code to help application recover (if possible). Misaligned address access depends on type of underlying block device. You have a good point, I will check how SD driver behaves as I don’t remember on top of my mind. But device should terminate gracefully even in case of unaligned access with error and not crash suddenly.

Every check we add increases code size. At some point we must trust the user is using our APIs correctly. I would consider a hard fault at the site of dereference a rather graceful error compared to other bugs they could run into with this API, ie buffer overruns in file_read.

Strongly agree with @geky here. If you're going to check NULL pointers, you logically need to be checking for misaligned pointers, stale pointers, out-of-range pointers, corrupted data structures, so on and so forth.

If you are going to worry about the validity of pointers you need to be writing in something other than C or C++03.

Are you going to add if (this == NULL) checks to the top of every method too? You would logically have to in the model you advocate.

And what happens in practice if you do return an error code on an invalid pointer? The caller probably ignores it. So we get subtle effects like we've had with some Mutex::lock()s not working without anyone realising, causing hard to track-down problems.

Having said all that...

the NULL dereference can easily be found with a debugger.

pyOCD still can't show a backtrace on a Cortex-M HardFault, as far as I know. It's been like that for a few years now. Rather undermines our argument, if pyOCD is our preferred debugger.

pyOCD still can't show a backtrace on a Cortex-M HardFault

Oh, this would be a good thing to fix!

deepikabhavnani · 2018-05-15T22:05:48Z

features/filesystem/storagelite/source/StorageLiteFS.cpp

+
+int StorageLiteFS::remove(const char *filename)
+{
+    int ret = _stlite->remove((const uint8_t *) filename, strlen(filename) + 1);


Error handling/checking- filename

Disagree, but the casts suggest a type problem - you should be passing filenames around as char * pointers, just like any string. If you need to cast to uint8_t * it should be lower level than this.

It this is converting to an opaque key, the opaque key's type should probably be void * to make caller's lives easier. And I wouldn't have thought you needed to include the NUL byte as part of the key, if you're giving explicit key lengths.

Right. The idea was to have opaque keys. will change to void * and remove the null terminator.

deepikabhavnani · 2018-05-15T22:06:25Z

features/filesystem/storagelite/source/StorageLiteFS.cpp

+{
+    size_t file_size;
+    int ret = _stlite->get_file_size((const uint8_t *) name, strlen(name) + 1, file_size);
+    st->st_size = file_size;


Error handling/checking- name / st. Accessing NULL st here can cause crash.

deepikabhavnani · 2018-05-15T22:08:31Z

features/filesystem/storagelite/source/StorageLiteFS.cpp

+    strcpy(handle->file_name, path);
+    handle->open_flags = flags;
+
+    *file = handle;


Error handling - user can pass NULL to file pointer.

kjbracey · 2018-05-16T08:09:54Z

Agree that all the basic functions here - get/set/remove/get_file_size/get_next are perfectly standard filing system APIs and should be accessible via FileSystem and FileHandle (even if there is a more-specific KVStore API/wrapper).

There's no justification for not being able to do

FILE *settings_file = fopen("/slite/settings", "r");
if (settings_file) {
    fread(my_settings, 1, sizeof my_settings, settings_file);
    fclose(settings_file);
}

Apps shouldn't need to be written to a special custom API to do something that simple.

Certainly calling this a "filing system" is rather confusing given that it is not actually an mbed OS filing system.

offirko · 2018-05-16T11:24:43Z

features/filesystem/storagelite/source/StorageLiteFS.cpp

+        }
+    }
+
+    handle->file_name = new char[strlen(path) + 1];


would consider verifying strlen(path) is smaller than max file name size to prevent memory corruption or security problem

davidsaada · 2018-05-16T12:05:55Z

@geky @kjbracey-arm Many of your observations were discussed in the design phase, so not sure they should also be discussed here as well. Still here's my take:

StorageLite is not a file system:
It is indeed not a file system. It is a key value store with security characteristics and additional features (such as factory reset). As such, it includes APIs to support key value store - e.g. get, set, remove etc.
So why the POSIX APIs?
POSIX APIs were added later on upon requirement, for users who wanted a well known interface without porting issues.
Now you got me confused. If it has POSIX APIs, it is a file system, isn't it?
Not quite. Many of the file system operations are not supported. Operations like seek, directory manipulations, renaming files and more. In fact, it only supports the open-read-close and open-write-close sequences. Anything else will fail.
Oh. Why these restrictions?
StorageLite is designed to run on Flash devices. Although, like all other file systems, it runs on top of a block device, this block device must have flash attributes. Implementing a fully operational filesystem would have a great performance penalty if you only need key value store operations. In fact, we have a fully functional file system on to of Flash, called LittleFS. However, simulating a standard client provisioning action (writing 30 files of 512B each) takes less than a second in StorageLite and more than 8 seconds in LittleFS. In addition, Flash wear leveling in StorageLite is optimal - it only erases sectors after the whole file system space is exhausted, something that can't be achieved with a regular file system.
OK, we can live with these restrictions. Why not use only the POSIX APIs and discard the other ones (get/set etc.)?
Not so fast. POSIX APIs don't offer the full functionality StorageLite offers, as they have a standard restricted interface. These include security features such as encryption and rollback protection on a per file basis, as well as factory reset. Security features are a must. Factory reset is required by the client. In addition, imitating the POSIX API behavior can also have a performance penalty. For instance, the open-read-close operation takes twice as much time comparing to the get operation. This is because when we open a file for reading, we need to check that the file exists on flash, aside from the read operation. Applications that don't need the standard APIs (like the client) would hence prefer using the get/set APIs over the POSIX ones.
You can do better than that! Why not separate the additional features from the basic storage operations and implement it as separate layers?
We can, but again - it will have a great (negative) impact on performance and functionality. Factory reset, for one, will need to be implemented as a separate set of files (with different names), holding a lot of space in RAM and flash. As for the security features, trying to implement them (encryption & rollback protection) in the block device layer produced horrendous performance. Implementing these on top of the storage layer will mean that we will also have to separate security actions from storage actions, while in StorageLite they are combined. The file header, for instance, holds both security and storage fields. Separating those will waste storage space. Furthermore, this separation will lose the atomicity of operations. One example would be writing a large encrypted file. This is done in one shot in StorageLite. How would this be achieved after the separation? encrypt the entire file in memory? separate the file to smaller segments in storage? and if so, how do you link these segments or handle extreme cases (like reaching the end of our area if it happens)?

kjbracey · 2018-05-16T15:06:11Z

There may a reason for an "optimised" custom API. If this were an entirely internal component written just for the purposes of mbed-client, and you were prepared to put the necessary work into its "storage abstraction layer", then it would be fine to stop there.

But that's still no reason to not support the standard API. There is nothing particularly unusual about a filing system with your restrictions, and there are many out there. And the majority of applications will work perfectly fine on them.

LTFS is a good example - it's a tape filing system, so has all the usual restrictions of tape - no random access, no reclaiming of space when a file is deleted until the tape is reformatted.

But it works fine for a lot of purposes with no custom code by just presenting itself as a standard POSIX filing system. Yes, dragging a load of files off a tape from Windows filer window can be inefficient, because it copies in alphabetical order rather than tape order, causing unnecessary seeking, but it works. And it doesn't preclude someone using a custom "tape-aware" program to do it better.

So even if the native interface remains as it stands, it should be an urgent priority to "complete the job" by providing an actual StorageLiteFS that implements a real FilingSystem on top of StorageLite so normal programs can just use it without having to invent their own "storage abstraction layer". This could be a separate wrapper - I doubt it would need to touch the internals.

And on atomicity, I was assuming the atomicity would be achieved simplistically by buffering the operation in the FileHandle - so the set actually happens on close and the get actually happens on open.

I'm not aware of a demand to manipulate excessively large items. If they were that large it would be equally unreasonable to expect them to be in a contiguous application buffer for the "set" or "get" as you currently do. Providing the POSIX interface could in some cases just move the required buffer from the app into the FileHandle, without increasing the total RAM.

geky · 2018-05-16T16:35:01Z

@davidsaada, I like your summary. However, I disagree with some of your assumptions:

Implementing a fully operational filesystem [on flash] would have a great performance penalty if you only need key value store operations.

That's just not true. StorageLite's optimizations come from minimizing erases and tracking file locations in RAM. Nothing related to the API.

And comparing to LittleFS doesn't prove what's possible. LittleFS was designed for RAM/ROM consumption, not for speed.
Flash wear leveling in StorageLite is optimal - it only erases sectors after the whole file system space is exhausted, something that can't be achieved with a regular file system.

That's not true either. Look at SPIFFS. It is a fully features POSIX file system with static wear leveling.

Keep in mind static wear leveling has its own set of issues in the form of wear amplification.
POSIX APIs don't offer the full functionality StorageLite offers, as they have a standard restricted interface.

You can add features without giving up on the API. Look at all the block devices, they get along pretty well together.
imitating the POSIX API behavior can also have a performance penalty. For instance, the open-read-close operation takes twice as much time comparing to the get operation.

So fix it. At this point you only need to authenticate the name, not also the rest of the file contents in open.
Factory reset, for one, will need to be implemented as a separate set of files (with different names), holding a lot of space in RAM and flash.

Use a special name for files that share data. Or add support for reflink, as a COW filesystem LittleFS could handle this very efficiently.
Trying to implement them (encryption & rollback protection) in the block device layer produced horrendous performance

If you don't care about rollback protection, encrypting the block device was showing only a ~1.4x slowdown vs encrypting the filesystem. Not unreasonable given the ~0.8x RAM/ROM improvement.
The file header, for instance, holds both security and storage fields. Separating those will waste storage space.

Separating things don't make them larger.
Furthermore, this separation will lose the atomicity of operations.

Sounds like an API problem. Incremental writes to POSIX files would allow you to update the data atomically.

If this needs to be applied to the KVStore API, consider adding user provided custom attributes. There's been discussions about adding these to LittleFS: Manage generic attributes littlefs-project/littlefs#23 (comment). This would allow you to build features on top of storage.
One example would be writing a large encrypted file. This is done in one shot in StorageLite. How would this be achieved after the separation? encrypt the entire file in memory?

Isn't this currently how StorageLite works? StorageLite files must always be stored entirely in memory. We should not be encouraging the use of StorageLite with large files.

That's the biggest downside of the KVStore API, someone has to cough up the RAM somewhere.

davidsaada · 2018-05-17T14:44:55Z

Pushed a commit with a few changes suggested in the comments. Mainly trying to handle build issues.
(Sorry for squashing, was unintentional).

dannybenor · 2018-05-17T15:31:17Z

I strongly recommend to limit the PR review to code and not feature or product definitions.
Storage lite provides a limited POSIX API as required in the design after reviewed by tech leaders.
The limitations are not an accident but a result of design and analysis.

adbridge · 2018-05-21T13:24:39Z

@geky @kjbracey-arm @deepikabhavnani are you happy with the updates to this PR ?

davidsaada · 2018-05-21T14:08:37Z

@adbridge From my POV, main gaps I still need to fill are the additional tests (resilience and wear level) and fixing the README according to the comments.

geky

+1 for the additional tests (resilience and wear level)
At minimum we still need a KVStore interface (abstract base class) for the new API that StorageLite implements.

And I still have problems with the design as stated above, this is being reviewed internally.

cmonr · 2018-05-21T15:57:55Z

Marking as DNM due to ongoing internal review.

@simonfordarm

1. Return errors when attempting to erase a worn out erase unit or write to it. 2. Support erase of more than one erase unit (in one call).

davidsaada · 2018-06-02T05:56:54Z

(as it's a post release weekend)
/morph build

mbed-ci · 2018-06-02T06:46:14Z

Build : SUCCESS

Build number : 2225
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6915/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-06-02T11:29:14Z

Test : FAILURE

Build number : 2016
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6915/2016

mbed-ci · 2018-06-02T15:16:03Z

Exporter Build : SUCCESS

Build number : 1853
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6915/

cmonr · 2018-06-04T16:10:27Z

From our (@ARMmbed/mbed-os-maintainers) understanding, StorageLite requires a redesign before it can be reintroduced into Mbed OS. Since we don't support/allow PRs to remain open for development purposes, I'm going to close this PR.

davidsaada force-pushed the david_storagelite branch from e6fefd0 to 368611a Compare May 15, 2018 15:45

davidsaada force-pushed the david_storagelite branch 4 times, most recently from 5e92b01 to 0a55174 Compare May 15, 2018 16:50

cmonr requested review from geky, sg-, andresag01 and a team May 15, 2018 16:58

cmonr added the needs: review label May 15, 2018

geky suggested changes May 15, 2018

View reviewed changes

geky added the scope: feature label May 15, 2018

deepikabhavnani reviewed May 15, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: review labels May 16, 2018

offirko reviewed May 16, 2018

View reviewed changes

cmonr removed the request for review from andresag01 May 16, 2018 19:00

cmonr added the release-version: 5.9.0-rc1 label May 17, 2018

davidsaada force-pushed the david_storagelite branch from 0a55174 to b15bdf3 Compare May 17, 2018 14:41

davidsaada force-pushed the david_storagelite branch from b15bdf3 to 690ad5e Compare May 17, 2018 14:57

davidsaada force-pushed the david_storagelite branch from 690ad5e to 7aa5f78 Compare May 17, 2018 15:06

davidsaada force-pushed the david_storagelite branch 3 times, most recently from 1dccac7 to ffb3d18 Compare May 21, 2018 09:41

kjbracey previously approved these changes May 21, 2018

View reviewed changes

geky suggested changes May 21, 2018

View reviewed changes

cmonr added the do not merge label May 21, 2018

0xc0170 removed the release-version: 5.9.0-rc1 label May 23, 2018

Fix two issues in ExhaustibleBlockDevice:

831c36d

1. Return errors when attempting to erase a worn out erase unit or write to it. 2. Support erase of more than one erase unit (in one call).

davidsaada dismissed kjbracey’s stale review via baee890 May 30, 2018 16:41

davidsaada force-pushed the david_storagelite branch 6 times, most recently from fa20816 to cfaaa70 Compare June 1, 2018 19:44

Add StorageLite feature

1234a0b

davidsaada force-pushed the david_storagelite branch from cfaaa70 to 1234a0b Compare June 1, 2018 20:31

cmonr closed this Jun 4, 2018

Add the StorageLite feature #6915

Add the StorageLite feature #6915

Uh oh!

Conversation

davidsaada commented May 15, 2018

Description

Pull request type

Uh oh!

davidsaada commented May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented May 15, 2018

Uh oh!

geky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidsaada commented May 15, 2018 •

edited

Loading

geky May 16, 2018 •

edited

Loading