Fix Memory Leaks / fix Dead Locks / correct mbrBlockDevice behaviour when calling begin(),umount(),format()

[ddmesh]

I had a lot of instabilities while testing and evaluating this library and found a lot of issues that should be fixed in main stream.

• fix several memory leaks and freeing arrays
• rework member variables when calling begin/umount/format (solves inconsistencies). begin() means mounting. It must not initialize the block device, as this is global to InternalStorage object. format() is only possible when FS is not mounted (it makes no sense to call begin()/unmount(), just to get a valid mbr block device). It created conflicts when calling format()/umount() and do memory freeing.
• When creating InternalStorage object and no partition was present on block device, the members of InternalStorage object left uninitialized.
• add new function freePartitionName() to free the allocated memory that was returned when calling utils function createPartitionName().
• solve problems, where block devices kept initialized when calling listPartitions(). This function could only be called two times. Other called functions failed (deadlock) because of the blocked resources underneath. It now calls deinit() to leave it clean.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[github-actions]

Memory usage change @ ef49605

Board	flash	%	RAM for global variables	%
arduino:mbed_opta:opta	🔺 +176 - +184	+0.01 - +0.01	0 - 0	0.0 - 0.0
arduino:mbed_portenta:envie_m7	N/A	N/A	N/A	N/A
arduino:renesas_portenta:portenta_c33	🔺 +128 - +200	+0.01 - +0.01	0 - 0	0.0 - 0.0

Click for full report table

Board	extras/tests/TestExisting flash	%	extras/tests/TestExisting RAM for global variables	%	extras/tests/TestFileOperations flash	%	extras/tests/TestFileOperations RAM for global variables	%	extras/tests/TestFolderOperations flash	%	extras/tests/TestFolderOperations RAM for global variables	%	extras/tests/TestRepeatedFormatMount flash	%	extras/tests/TestRepeatedFormatMount RAM for global variables	%	examples/SimpleStorageWriteRead flash	%	examples/SimpleStorageWriteRead RAM for global variables	%	examples/AdvancedUSBInternalOperations flash	%	examples/AdvancedUSBInternalOperations RAM for global variables	%	examples/BackupInternalPartitions flash	%	examples/BackupInternalPartitions RAM for global variables	%
arduino:mbed_opta:opta	184	0.01	0	0.0	184	0.01	0	0.0	184	0.01	0	0.0	176	0.01	0	0.0	184	0.01	0	0.0	184	0.01	0	0.0	184	0.01	0	0.0
arduino:mbed_portenta:envie_m7	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A
arduino:renesas_portenta:portenta_c33	200	0.01	0	0.0	184	0.01	0	0.0	192	0.01	0	0.0	192	0.01	0	0.0	192	0.01	0	0.0	200	0.01	0	0.0	128	0.01	0	0.0

Click for full report CSV

Board,extras/tests/TestExisting<br>flash,%,extras/tests/TestExisting<br>RAM for global variables,%,extras/tests/TestFileOperations<br>flash,%,extras/tests/TestFileOperations<br>RAM for global variables,%,extras/tests/TestFolderOperations<br>flash,%,extras/tests/TestFolderOperations<br>RAM for global variables,%,extras/tests/TestRepeatedFormatMount<br>flash,%,extras/tests/TestRepeatedFormatMount<br>RAM for global variables,%,examples/SimpleStorageWriteRead<br>flash,%,examples/SimpleStorageWriteRead<br>RAM for global variables,%,examples/AdvancedUSBInternalOperations<br>flash,%,examples/AdvancedUSBInternalOperations<br>RAM for global variables,%,examples/BackupInternalPartitions<br>flash,%,examples/BackupInternalPartitions<br>RAM for global variables,%
arduino:mbed_opta:opta,184,0.01,0,0.0,184,0.01,0,0.0,184,0.01,0,0.0,176,0.01,0,0.0,184,0.01,0,0.0,184,0.01,0,0.0,184,0.01,0,0.0
arduino:mbed_portenta:envie_m7,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A
arduino:renesas_portenta:portenta_c33,200,0.01,0,0.0,184,0.01,0,0.0,192,0.01,0,0.0,192,0.01,0,0.0,192,0.01,0,0.0,200,0.01,0,0.0,128,0.01,0,0.0

[sebromero]

Thanks a lot @ddmesh for your contribution! We'll take a look at your PR shortly 👀

[facchinm]

Hi @ddmesh, thanks for the patch, but in its current state it is unmergeable.
I would kindly ask you to split into different commits with clean scope (for example, one for typos, one for functionalities, one for bugfixes) without unrelated whitespace changes, all with a clear description of what they are doing.

[facchinm]

Need to split into multiple commits to make code review feasible

[ddmesh]

Hi, I can not split this commit into several. There are not so much changes and those should be checked easily. Most code editors like VSCode will remove spaces automatically to avoid more visible changes in future in source code. So it is always a good idea to never check-in code that has spaces at line end.

Please compare the code with spaces ignored.
We have our own git repository in our company where I have fixed those issues. I just wanted to give those fixes back and created a commit for you.
I took me already quite a while to analyse and test the code after I run into unexpected issues. I really would appreciate that you verify and check the merge request as it is. I can not separate this commit (no time).

Git CI tells that all tests are successful, only one reviewer is needed. The code is mergable, as there are no merge conflicts. All changes are on top on main branch.

[facchinm]

@ddmesh can you sign the CLA please? I'll then proceed in splitting the PR into sensible commits to keep the actual patch understandable for future readers 🙂

[aliphys]

This fix is also included in #44 as a single commit

[ddmesh]

@ddmesh can you sign the CLA please? I'll then proceed in splitting the PR into sensible commits to keep the actual patch understandable for future readers 🙂

hi, thanks a lot for splitting. I have signed the cla several times and it says that I already have it. but the state here is not updated. on cla website it is also listed as "signed". seems GitHub integration is not working correctly.
BR Stephan

[facchinm]

I think the CLA is failing due to the committer being dummy@github.com . I splitted between whitespaces fixes and actual code, and I noticed a thing that don't sound completely right:

• calling this -> blockDevice = BlockDeviceType::get_default_instance(); this -> mbrBlockDevice = new MBRBlockDeviceType(this -> blockDevice, this->partitionNumber); from the constructor can have unintended consequences due to the fact that the constructors might be called in any order. Why did you move it from begin()?

The other modifications look sensible, anyway I'd ask @aliphys and @cristidragomir97 to give it a spin before merging

[ddmesh]

I moved the creation/initialisation from begin to constructor because the blockdevice is global to "InternalStorage" and does not change at all over time. Only when the InternalStorage object is destroyed the device below becomes released.
begin() is used to mount a file system. The user can umount/mount his blockdevice as he likes several times. For instance do an umount/format/mount. The whole time the block device should stay initalised and must not initialised more then once. If you always destroy and reinit the underlaying blockdevice than you waste a lot of time/resources which can be easily avoided.

There is a 1:1 relation how long InternalStorage object lives and the lifetime of underlaying blockdevice.

When user creates an instance of InternalStorage globally, you are right, that the order when a constructor is called is undefined. But a conflict with the underlaying blockdevice (e.g. QSPI flash) should not happen at all, because only one "owner" must initialise the underlaying blockdevice. Using different APIs or create more than one instance are a programming error. When you initialize the underlaying blockdevice in begin() you will have the same issue and more, beause you reinitialize the blockdevice several times without freeing it (memleak and deadlock).
If you want to protect the InternalStorage object, you must create a singleton , so only one instance of InternalStorage can be created at all.

[github-actions]

Memory usage change @ bd8600c

Board	flash	%	RAM for global variables	%
arduino:mbed_opta:opta	🔺 +192 - +192	+0.01 - +0.01	0 - 0	0.0 - 0.0
arduino:mbed_portenta:envie_m7	N/A	N/A	N/A	N/A
arduino:renesas_portenta:portenta_c33	🔺 +136 - +208	+0.01 - +0.01	0 - 0	0.0 - 0.0

Click for full report table

Board	extras/tests/TestExisting flash	%	extras/tests/TestExisting RAM for global variables	%	extras/tests/TestFileOperations flash	%	extras/tests/TestFileOperations RAM for global variables	%	extras/tests/TestFolderOperations flash	%	extras/tests/TestFolderOperations RAM for global variables	%	extras/tests/TestRepeatedFormatMount flash	%	extras/tests/TestRepeatedFormatMount RAM for global variables	%	examples/SimpleStorageWriteRead flash	%	examples/SimpleStorageWriteRead RAM for global variables	%	examples/AdvancedUSBInternalOperations flash	%	examples/AdvancedUSBInternalOperations RAM for global variables	%	examples/BackupInternalPartitions flash	%	examples/BackupInternalPartitions RAM for global variables	%
arduino:mbed_opta:opta	192	0.01	0	0.0	192	0.01	0	0.0	192	0.01	0	0.0	192	0.01	0	0.0	192	0.01	0	0.0	192	0.01	0	0.0	192	0.01	0	0.0
arduino:mbed_portenta:envie_m7	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A
arduino:renesas_portenta:portenta_c33	208	0.01	0	0.0	192	0.01	0	0.0	200	0.01	0	0.0	200	0.01	0	0.0	200	0.01	0	0.0	208	0.01	0	0.0	136	0.01	0	0.0

Click for full report CSV

Board,extras/tests/TestExisting<br>flash,%,extras/tests/TestExisting<br>RAM for global variables,%,extras/tests/TestFileOperations<br>flash,%,extras/tests/TestFileOperations<br>RAM for global variables,%,extras/tests/TestFolderOperations<br>flash,%,extras/tests/TestFolderOperations<br>RAM for global variables,%,extras/tests/TestRepeatedFormatMount<br>flash,%,extras/tests/TestRepeatedFormatMount<br>RAM for global variables,%,examples/SimpleStorageWriteRead<br>flash,%,examples/SimpleStorageWriteRead<br>RAM for global variables,%,examples/AdvancedUSBInternalOperations<br>flash,%,examples/AdvancedUSBInternalOperations<br>RAM for global variables,%,examples/BackupInternalPartitions<br>flash,%,examples/BackupInternalPartitions<br>RAM for global variables,%
arduino:mbed_opta:opta,192,0.01,0,0.0,192,0.01,0,0.0,192,0.01,0,0.0,192,0.01,0,0.0,192,0.01,0,0.0,192,0.01,0,0.0,192,0.01,0,0.0
arduino:mbed_portenta:envie_m7,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A,N/A
arduino:renesas_portenta:portenta_c33,208,0.01,0,0.0,192,0.01,0,0.0,200,0.01,0,0.0,200,0.01,0,0.0,200,0.01,0,0.0,208,0.01,0,0.0,136,0.01,0,0.0

[ddmesh]

About the CLA, I tried to change "amend" the author from last commit, but his was ignored by github after pushing it (if this is the reason why the status is still "pending").
Any Idea?

beside of this, when commiting and pushing to github, github replaces the email address anyway to protect privacy. Perhaps there is another reason why this is pending?