summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDavid Kalnischkies <david@kalnischkies.de>2017-05-28 22:26:17 +0200
committerDavid Kalnischkies <david@kalnischkies.de>2017-06-26 23:31:15 +0200
commit188f297a2af4c15cb1d502360d1e478644b5b810 (patch)
tree4063f8442876359787efe907ac9dbdd86d97fdea
parentd7c92411dc1f4c6be098d1425f9c1c075e0c2154 (diff)
show .diff/Index properly as ignored if we fallback
Moving the code responsible for parsing the Index file from ::Done into the slightly earlier ::VerifyDone allows us to still "fail" the download if we can't make use of the Index for whatever reason, so that the progress log correctly displays "Ign" instead of "Get" for the file. This also makes quiet a few debug messages proper error messages (but those are still hidden by default for Ign lines).
-rw-r--r--apt-pkg/acquire-item.cc170
-rw-r--r--apt-pkg/acquire-item.h31
-rwxr-xr-xtest/integration/test-pdiff-usage4
3 files changed, 94 insertions, 111 deletions
diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index 17e4797d1..3ce0f25cf 100644
--- a/apt-pkg/acquire-item.cc
+++ b/apt-pkg/acquire-item.cc
@@ -53,18 +53,6 @@
using namespace std;
-static void printHashSumComparison(std::string const &URI, HashStringList const &Expected, HashStringList const &Actual) /*{{{*/
-{
- if (_config->FindB("Debug::Acquire::HashSumMismatch", false) == false)
- return;
- std::cerr << std::endl << URI << ":" << std::endl << " Expected Hash: " << std::endl;
- for (HashStringList::const_iterator hs = Expected.begin(); hs != Expected.end(); ++hs)
- std::cerr << "\t- " << hs->toStr() << std::endl;
- std::cerr << " Actual Hash: " << std::endl;
- for (HashStringList::const_iterator hs = Actual.begin(); hs != Actual.end(); ++hs)
- std::cerr << "\t- " << hs->toStr() << std::endl;
-}
- /*}}}*/
static std::string GetPartialFileName(std::string const &file) /*{{{*/
{
std::string DestFile = _config->FindDir("Dir::State::lists") + "partial/";
@@ -1675,7 +1663,8 @@ void pkgAcqMetaClearSig::Finished() /*{{{*/
bool pkgAcqMetaClearSig::VerifyDone(std::string const &Message, /*{{{*/
pkgAcquire::MethodConfig const * const Cnf)
{
- Item::VerifyDone(Message, Cnf);
+ if (Item::VerifyDone(Message, Cnf) == false)
+ return false;
if (FileExists(DestFile) && !StartsWithGPGClearTextSignature(DestFile))
return RenameOnError(NotClearsigned);
@@ -2051,13 +2040,11 @@ void pkgAcqDiffIndex::QueueOnIMSHit() const /*{{{*/
new pkgAcqIndexDiffs(Owner, TransactionManager, Target, UsedMirror, Target.URI);
}
/*}}}*/
-static bool RemoveFileForBootstrapLinking(bool const Debug, std::string const &For, std::string const &Boot)/*{{{*/
+static bool RemoveFileForBootstrapLinking(std::string &ErrorText, std::string const &For, std::string const &Boot)/*{{{*/
{
if (FileExists(Boot) && RemoveFile("Bootstrap-linking", Boot) == false)
{
- if (Debug)
- std::clog << "Bootstrap-linking for patching " << For
- << " by removing stale " << Boot << " failed!" << std::endl;
+ strprintf(ErrorText, "Bootstrap for patching %s by removing stale %s failed!", For.c_str(), Boot.c_str());
return false;
}
return true;
@@ -2065,6 +2052,7 @@ static bool RemoveFileForBootstrapLinking(bool const Debug, std::string const &F
/*}}}*/
bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile) /*{{{*/
{
+ available_patches.clear();
ExpectedAdditionalItems = 0;
// failing here is fine: our caller will take care of trying to
// get the complete file if patching fails
@@ -2108,8 +2096,7 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile) /*{{{*/
if (ServerHashes.usable() == false)
{
- if (Debug == true)
- std::clog << "pkgAcqDiffIndex: " << IndexDiffFile << ": Did not find a good hashsum in the index" << std::endl;
+ ErrorText = "Did not find a good hashsum in the index";
return false;
}
@@ -2117,11 +2104,7 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile) /*{{{*/
HashStringList const TargetFileHashes = GetExpectedHashesFor(Target.MetaKey);
if (TargetFileHashes.usable() == false || ServerHashes != TargetFileHashes)
{
- if (Debug == true)
- {
- std::clog << "pkgAcqDiffIndex: " << IndexDiffFile << ": Index has different hashes than parser, probably older, so fail pdiffing" << std::endl;
- printHashSumComparison(CurrentPackagesFile, ServerHashes, TargetFileHashes);
- }
+ ErrorText = "Index has different hashes than parser (probably older)";
return false;
}
@@ -2139,10 +2122,7 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile) /*{{{*/
if (ServerHashes == LocalHashes)
{
- // we have the same sha1 as the server so we are done here
- if(Debug)
- std::clog << "pkgAcqDiffIndex: Package file " << CurrentPackagesFile << " is up-to-date" << std::endl;
- QueueOnIMSHit();
+ available_patches.clear();
return true;
}
@@ -2159,7 +2139,6 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile) /*{{{*/
types.push_back(*type);
// parse all of (provided) history
- vector<DiffInfo> available_patches;
bool firstAcceptedHashes = true;
for (auto type = types.crbegin(); type != types.crend(); ++type)
{
@@ -2214,9 +2193,7 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile) /*{{{*/
if (unlikely(available_patches.empty() == true))
{
- if (Debug)
- std::clog << "pkgAcqDiffIndex: " << IndexDiffFile << ": "
- << "Couldn't find any patches for the patch series." << std::endl;
+ ErrorText = "Couldn't find any patches for the patch series";
return false;
}
@@ -2318,9 +2295,7 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile) /*{{{*/
if (foundStart == false || unlikely(available_patches.empty() == true))
{
- if (Debug)
- std::clog << "pkgAcqDiffIndex: " << IndexDiffFile << ": "
- << "Couldn't find the start of the patch series." << std::endl;
+ ErrorText = "Couldn't find the start of the patch series";
return false;
}
@@ -2329,9 +2304,7 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile) /*{{{*/
patch.patch_hashes.usable() == false ||
patch.download_hashes.usable() == false)
{
- if (Debug)
- std::clog << "pkgAcqDiffIndex: " << IndexDiffFile << ": provides no usable hashes for " << patch.file
- << " so fallback to complete download" << std::endl;
+ strprintf(ErrorText, "Provides no usable hashes for %s", patch.file.c_str());
return false;
}
@@ -2339,9 +2312,7 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile) /*{{{*/
unsigned long const fileLimit = _config->FindI("Acquire::PDiffs::FileLimit", 0);
if (fileLimit != 0 && fileLimit < available_patches.size())
{
- if (Debug)
- std::clog << "Need " << available_patches.size() << " diffs (Limit is " << fileLimit
- << ") so fallback to complete download" << std::endl;
+ strprintf(ErrorText, "Need %lu diffs, but limit is %lu", available_patches.size(), fileLimit);
return false;
}
@@ -2371,25 +2342,18 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile) /*{{{*/
unsigned long long const sizeLimit = downloadSizeIdx * sizeLimitPercent;
if ((sizeLimit/100) < downloadSize)
{
- if (Debug)
- std::clog << "Need " << downloadSize << " compressed bytes (Limit is " << (sizeLimit/100) << ", "
- << "original is " << downloadSizeIdx << ") so fallback to complete download" << std::endl;
+ strprintf(ErrorText, "Need %llu compressed bytes, but limit is %llu and original is %llu", downloadSize, (sizeLimit/100), downloadSizeIdx);
return false;
}
}
}
- // we have something, queue the diffs
- string::size_type const last_space = Description.rfind(" ");
- if(last_space != string::npos)
- Description.erase(last_space, Description.size()-last_space);
-
/* decide if we should download patches one by one or in one go:
The first is good if the server merges patches, but many don't so client
based merging can be attempt in which case the second is better.
"bad things" will happen if patches are merged on the server,
but client side merging is attempt as well */
- bool pdiff_merge = _config->FindB("Acquire::PDiffs::Merge", true);
+ pdiff_merge = _config->FindB("Acquire::PDiffs::Merge", true);
if (pdiff_merge == true)
{
// reprepro adds this flag if it has merged patches on the server
@@ -2404,53 +2368,24 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string const &IndexDiffFile) /*{{{*/
return false;
std::string const PartialFile = GetPartialFileNameFromURI(Target.URI);
std::string const PatchedFile = GetKeepCompressedFileName(PartialFile + "-patched", Target);
- if (RemoveFileForBootstrapLinking(Debug, CurrentPackagesFile, PartialFile) == false ||
- RemoveFileForBootstrapLinking(Debug, CurrentPackagesFile, PatchedFile) == false)
+ if (RemoveFileForBootstrapLinking(ErrorText, CurrentPackagesFile, PartialFile) == false ||
+ RemoveFileForBootstrapLinking(ErrorText, CurrentPackagesFile, PatchedFile) == false)
return false;
for (auto const &ext : APT::Configuration::getCompressorExtensions())
{
- if (RemoveFileForBootstrapLinking(Debug, CurrentPackagesFile, PartialFile + ext) == false ||
- RemoveFileForBootstrapLinking(Debug, CurrentPackagesFile, PatchedFile + ext) == false)
+ if (RemoveFileForBootstrapLinking(ErrorText, CurrentPackagesFile, PartialFile + ext) == false ||
+ RemoveFileForBootstrapLinking(ErrorText, CurrentPackagesFile, PatchedFile + ext) == false)
return false;
}
std::string const Ext = Final.substr(CurrentPackagesFile.length());
std::string const Partial = PartialFile + Ext;
if (symlink(Final.c_str(), Partial.c_str()) != 0)
{
- if (Debug)
- std::clog << "Bootstrap-linking for patching " << CurrentPackagesFile
- << " by linking " << Final << " to " << Partial << " failed!" << std::endl;
+ strprintf(ErrorText, "Bootstrap for patching by linking %s to %s failed!", Final.c_str(), Partial.c_str());
return false;
}
}
- std::string indexURI = Desc.URI;
- auto const byhashidx = indexURI.find("/by-hash/");
- if (byhashidx != std::string::npos)
- indexURI = indexURI.substr(0, byhashidx - strlen(".diff"));
- else
- {
- auto end = indexURI.length() - strlen(".diff/Index");
- if (CurrentCompressionExtension != "uncompressed")
- end -= (1 + CurrentCompressionExtension.length());
- indexURI = indexURI.substr(0, end);
- }
-
- if (pdiff_merge == false)
- new pkgAcqIndexDiffs(Owner, TransactionManager, Target, UsedMirror, indexURI, available_patches);
- else
- {
- diffs = new std::vector<pkgAcqIndexMergeDiffs*>(available_patches.size());
- for(size_t i = 0; i < available_patches.size(); ++i)
- (*diffs)[i] = new pkgAcqIndexMergeDiffs(Owner, TransactionManager,
- Target, UsedMirror, indexURI,
- available_patches[i],
- diffs);
- }
-
- Complete = false;
- Status = StatDone;
- Dequeue();
return true;
}
/*}}}*/
@@ -2462,6 +2397,10 @@ void pkgAcqDiffIndex::Failed(string const &Message,pkgAcquire::MethodConfig cons
Status = StatDone;
ExpectedAdditionalItems = 0;
+ // queue for final move - this should happen even if we fail
+ // while parsing (e.g. on sizelimit) and download the complete file.
+ TransactionManager->TransactionStageCopy(this, DestFile, GetFinalFilename());
+
if(Debug)
std::clog << "pkgAcqDiffIndex failed: " << Desc.URI << " with " << Message << std::endl
<< "Falling back to normal index file acquire" << std::endl;
@@ -2469,6 +2408,21 @@ void pkgAcqDiffIndex::Failed(string const &Message,pkgAcquire::MethodConfig cons
new pkgAcqIndex(Owner, TransactionManager, Target);
}
/*}}}*/
+bool pkgAcqDiffIndex::VerifyDone(std::string const &Message, pkgAcquire::MethodConfig const * const)/*{{{*/
+{
+ string const FinalFile = GetFinalFilename();
+ if(StringToBool(LookupTag(Message,"IMS-Hit"),false))
+ DestFile = FinalFile;
+
+ if (ParseDiffIndex(DestFile))
+ return true;
+
+ Status = StatError;
+ if (ErrorText.empty())
+ ErrorText = "Couldn't parse pdiff index";
+ return false;
+}
+ /*}}}*/
void pkgAcqDiffIndex::Done(string const &Message,HashStringList const &Hashes, /*{{{*/
pkgAcquire::MethodConfig const * const Cnf)
{
@@ -2477,20 +2431,46 @@ void pkgAcqDiffIndex::Done(string const &Message,HashStringList const &Hashes, /
Item::Done(Message, Hashes, Cnf);
- string const FinalFile = GetFinalFilename();
- if(StringToBool(LookupTag(Message,"IMS-Hit"),false))
- DestFile = FinalFile;
-
- if(ParseDiffIndex(DestFile) == false)
+ if (available_patches.empty())
{
- Failed("Message: Couldn't parse pdiff index", Cnf);
- // queue for final move - this should happen even if we fail
- // while parsing (e.g. on sizelimit) and download the complete file.
- TransactionManager->TransactionStageCopy(this, DestFile, FinalFile);
- return;
+ // we have the same sha1 as the server so we are done here
+ if(Debug)
+ std::clog << "pkgAcqDiffIndex: Package file is up-to-date" << std::endl;
+ QueueOnIMSHit();
+ }
+ else
+ {
+ // we have something, queue the diffs
+ string::size_type const last_space = Description.rfind(" ");
+ if(last_space != string::npos)
+ Description.erase(last_space, Description.size()-last_space);
+
+ std::string indexURI = Desc.URI;
+ auto const byhashidx = indexURI.find("/by-hash/");
+ if (byhashidx != std::string::npos)
+ indexURI = indexURI.substr(0, byhashidx - strlen(".diff"));
+ else
+ {
+ auto end = indexURI.length() - strlen(".diff/Index");
+ if (CurrentCompressionExtension != "uncompressed")
+ end -= (1 + CurrentCompressionExtension.length());
+ indexURI = indexURI.substr(0, end);
+ }
+
+ if (pdiff_merge == false)
+ new pkgAcqIndexDiffs(Owner, TransactionManager, Target, UsedMirror, indexURI, available_patches);
+ else
+ {
+ diffs = new std::vector<pkgAcqIndexMergeDiffs*>(available_patches.size());
+ for(size_t i = 0; i < available_patches.size(); ++i)
+ (*diffs)[i] = new pkgAcqIndexMergeDiffs(Owner, TransactionManager,
+ Target, UsedMirror, indexURI,
+ available_patches[i],
+ diffs);
+ }
}
- TransactionManager->TransactionStageCopy(this, DestFile, FinalFile);
+ TransactionManager->TransactionStageCopy(this, DestFile, GetFinalFilename());
Complete = true;
Status = StatDone;
diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h
index 7741dbf22..343144122 100644
--- a/apt-pkg/acquire-item.h
+++ b/apt-pkg/acquire-item.h
@@ -686,6 +686,20 @@ class APT_HIDDEN pkgAcqIndex : public pkgAcqBaseIndex
std::string const &Message, pkgAcquire::MethodConfig const * const Cnf);
};
/*}}}*/
+struct APT_HIDDEN DiffInfo { /*{{{*/
+ /** The filename of the diff. */
+ std::string file;
+
+ /** The hashes of the file after the diff is applied */
+ HashStringList result_hashes;
+
+ /** The hashes of the diff */
+ HashStringList patch_hashes;
+
+ /** The hashes of the compressed diff */
+ HashStringList download_hashes;
+};
+ /*}}}*/
/** \brief An item that is responsible for fetching an index file of {{{
* package list diffs and starting the package list's download.
*
@@ -699,6 +713,8 @@ class APT_HIDDEN pkgAcqDiffIndex : public pkgAcqIndex
{
void * const d;
std::vector<pkgAcqIndexMergeDiffs*> * diffs;
+ std::vector<DiffInfo> available_patches;
+ bool pdiff_merge;
protected:
/** \brief If \b true, debugging information will be written to std::clog. */
@@ -718,6 +734,7 @@ class APT_HIDDEN pkgAcqDiffIndex : public pkgAcqIndex
public:
// Specialized action members
virtual void Failed(std::string const &Message, pkgAcquire::MethodConfig const * const Cnf) APT_OVERRIDE;
+ virtual bool VerifyDone(std::string const &Message, pkgAcquire::MethodConfig const * const Cnf) APT_OVERRIDE;
virtual void Done(std::string const &Message, HashStringList const &Hashes,
pkgAcquire::MethodConfig const * const Cnf) APT_OVERRIDE;
virtual std::string DescURI() const APT_OVERRIDE {return Target.URI + "Index";};
@@ -752,20 +769,6 @@ class APT_HIDDEN pkgAcqDiffIndex : public pkgAcqIndex
APT_HIDDEN void QueueOnIMSHit() const;
};
/*}}}*/
-struct APT_HIDDEN DiffInfo { /*{{{*/
- /** The filename of the diff. */
- std::string file;
-
- /** The hashes of the file after the diff is applied */
- HashStringList result_hashes;
-
- /** The hashes of the diff */
- HashStringList patch_hashes;
-
- /** The hashes of the compressed diff */
- HashStringList download_hashes;
-};
- /*}}}*/
/** \brief An item that is responsible for fetching client-merge patches {{{
* that need to be applied to a given package index file.
*
diff --git a/test/integration/test-pdiff-usage b/test/integration/test-pdiff-usage
index 430551fa4..e7b052d49 100755
--- a/test/integration/test-pdiff-usage
+++ b/test/integration/test-pdiff-usage
@@ -50,7 +50,7 @@ wasmergeused() {
if echo "$*" | grep -q -- '-o test::cannot-use-pdiff=1'; then
msgtest 'Check if pdiff was' 'not used'
cp -a rootdir/tmp/testsuccess.output rootdir/tmp/aptupdate.output
- testsuccess --nomsg grep "diff/Index with Message: Couldn't parse pdiff index" rootdir/tmp/aptupdate.output
+ testsuccess --nomsg grep "^Ign:.* Packages\.diff/Index" rootdir/tmp/aptupdate.output
return;
fi
@@ -336,7 +336,7 @@ SHA256-Download:
generatereleasefiles '+1hour'
signreleasefiles
wasmergeused "$@" -o test::cannot-use-pdiff=1
- testsuccess grep 'bytes (Limit is' rootdir/tmp/aptupdate.output
+ testsuccess grep 'bytes, but limit is' rootdir/tmp/aptupdate.output
testnopackage oldstuff
testsuccessequal "$(cat "${PKGFILE}-new")
" aptcache show apt newstuff