Fix for binary download resume #1571
Conversation
eu-siemann
requested review from
lbonn,
pattivacek,
mike-sul,
xcheng-here and
kbushgit
eu-siemann
force-pushed
the
fix/binary-resume
branch
from
8e4ac84 to
064f82c
Compare
| @@ -9,7 +9,7 @@ struct DownloadMetaStruct { | |||
| target{std::move(target_in)}, | |||
| token{token_in}, | |||
| progress_cb{std::move(progress_cb_in)} {} | |||
| uint64_t downloaded_length{0}; | |||
| uintmax_t downloaded_length{0}; | |||
There was a problem hiding this comment.
I get why size_t was wrong, but is uintmax_t really necessary (compared to uint64_t)?
There was a problem hiding this comment.
I had the same question, it seems that the reason is that the c++ filesystem api (also boost) returns this type for file sizes: https://en.cppreference.com/w/cpp/filesystem/file_size.
There was a problem hiding this comment.
It should be not necessary, just wanted to be consistent with return types from boost.
There was a problem hiding this comment.
Fair enough. Seems fine.
| @@ -56,7 +56,7 @@ void resume(const Uptane::Target& target) { | |||
| EXPECT_TRUE(res); | |||
| } | |||
|
|
|||
| void pause_and_die(const Uptane::Target& target) { | |||
| void try_and_die(const Uptane::Target& target, bool graceful) { | |||
|
Looks fine to me, we can merge it after fixing the minor format nit. |
Signed-off-by: Eugene Smirnov <[email protected]>
Signed-off-by: Eugene Smirnov <[email protected]>
For 32-bit arch SIZE_MAX == (2^32 - 1), which will break on files > 4 GB. Signed-off-by: Eugene Smirnov <[email protected]>
Signed-off-by: Eugene Smirnov <[email protected]>
eu-siemann
force-pushed
the
fix/binary-resume
branch
from
064f82c to
4865dbf
Compare
Codecov Report
@@ Coverage Diff @@
## master #1571 +/- ##
========================================
- Coverage 82.19% 82.1% -0.1%
========================================
Files 189 189
Lines 11859 11855 -4
========================================
- Hits 9748 9734 -14
- Misses 2111 2121 +10
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
It's weird, but the order of commits on the branch doesn't match the order on github. Commit 864bcf0 goes before f4db090, so it's easier to test that the following commits are fixing the failing test. On github 864bcf0 is shown as the last commit.
I've seen that happen before. Seems like a github bug. Anyway, LGTM, thanks!
This can be considered a hotfix for the issue with download resume after a nongraceful shutdown (OLPSUP-9401).
I plan to follow up this PR with another one, removing usage of target_images table for binary downloads and possibly moving this functionality to (Binary) package manager.