Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

Store image files on the filesystem #1091

Merged
merged 2 commits into from
Feb 19, 2019
Merged

Store image files on the filesystem #1091

merged 2 commits into from
Feb 19, 2019

Conversation

patriotyk
Copy link
Contributor

Signed-off-by: Serhiy Stetskovych [email protected]

@patriotyk patriotyk force-pushed the feat/sql_to_fs_targets branch 3 times, most recently from 4296801 to 943e440 Compare February 12, 2019 14:01
@codecov-io
Copy link

codecov-io commented Feb 12, 2019

Codecov Report

Merging #1091 into master will increase coverage by 0.06%.
The diff coverage is 81.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1091      +/-   ##
==========================================
+ Coverage   75.66%   75.72%   +0.06%     
==========================================
  Files         158      158              
  Lines        9430     9392      -38     
==========================================
- Hits         7135     7112      -23     
+ Misses       2295     2280      -15
Impacted Files Coverage Δ
src/libaktualizr/storage/sqlstorage.h 50% <ø> (ø) ⬆️
src/libaktualizr/storage/sqlstorage_base.h 100% <ø> (ø) ⬆️
src/libaktualizr/storage/invstorage.h 97.43% <ø> (ø) ⬆️
src/libaktualizr/uptane/fetcher.cc 96.9% <100%> (ø) ⬆️
src/libaktualizr/storage/sqlstorage.cc 73.04% <80.48%> (+0.61%) ⬆️
src/libaktualizr/storage/sql_utils.h 80.59% <0%> (+0.31%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 096e7b5...b08bce8. Read the comment docs.

@eu-siemann
Copy link
Contributor

It looks to me that we will enter unrecoverable state if the binary file disappears from the filesystem:
checkTargetFile() will return true as it finds the database entry, but subsequent SQLTargetRHandle constructor will throw.

@patriotyk patriotyk force-pushed the feat/sql_to_fs_targets branch from 943e440 to ec5890f Compare February 12, 2019 15:53
@eu-siemann
Copy link
Contributor

eu-siemann commented Feb 12, 2019

I'm also not so sure about storing the files under the user-provided filename. As there can be additional directories in the filename, we either risk to end up with hundreds of empty dirs, or need to implement some directory cleanup logic.
What do you think about having just one folder for all binaries and using hash as a filename?

@pattivacek
Copy link
Collaborator

What do you think about having just one folder for all binaries and using hash as a filename?

I think that is not a bad idea. However, TUF seems to disagree. See section 5.5.2. Uptane seems to be on the same page.

@eu-siemann
Copy link
Contributor

eu-siemann commented Feb 12, 2019

@patrickvacek

the client MUST write the file to non-volatile storage as FILENAME.EXT

Well, if we say that this requirement is about the name in a filesystem, then storing binary blobs in sqlite db is also wrong :)
I guess it's more about to be able to associate FILENAME.EXT with the corresponding binary data, and the rest is an implementation detail.

@pattivacek
Copy link
Collaborator

Well, if we say that this requirement is about the name in a filesystem, then storing binary blobs in sqlite db is also wrong :)

Indeed!

I guess it's more about to be able to associate FILENAME.EXT with the corresponding binary data, and the rest is an implementation detail.

Yes, I agree. I can't think of a good reason why storing the files by their hash would be a bad thing. I can see some benefit in using the names as provided for visibility and maybe maintaining a "nice" directory structure, but that could be as much of a curse as it could be a blessing, and dealing with special characters could be annoying, and no one is really supposed to be looking at the hard drive anyway, so the perceived benefit would be minimal at best.

@patriotyk patriotyk force-pushed the feat/sql_to_fs_targets branch 3 times, most recently from d52d722 to 2e6a2e5 Compare February 14, 2019 10:00
@patriotyk
Copy link
Contributor Author

Please check, should be fine now.

@patriotyk patriotyk added ready and removed not-ready labels Feb 14, 2019
@eu-siemann
Copy link
Contributor

Thanks, Serhiy! Looks fine to me.

patriotyk and others added 2 commits February 18, 2019 17:02
Signed-off-by: Serhiy Stetskovych <[email protected]>
Redundant declarations + one expressions simplification

Signed-off-by: Laurent Bonnans <[email protected]>
@lbonn lbonn force-pushed the feat/sql_to_fs_targets branch from 2e6a2e5 to b08bce8 Compare February 18, 2019 16:02
@lbonn
Copy link
Contributor

lbonn commented Feb 18, 2019

I've rebased the PR and added a small tweaks commit for the minor style issues we've found.

Copy link
Collaborator

@pattivacek pattivacek 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!

@lbonn lbonn merged commit d6c0f26 into master Feb 19, 2019
@lbonn lbonn deleted the feat/sql_to_fs_targets branch February 19, 2019 10:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants