wrap every unlink call to check for != /dev/null

Unlinking /dev/null is bad, we shouldn't do that. Also, we should print
at least a warning if we tried to unlink a file but didn't manage to
pull it of (ignoring the case were the file is /dev/null or doesn't
exist in the first place).

This got triggered by a relatively unlikely to cause problem in
pkgAcquire::Worker::PrepareFiles which would while temporary
uncompressed files (which are set to keep compressed) figure out that to
files are the same and prepare for sharing by deleting them. Bad move.
That also shows why not printing a warning is a bad idea as this hide
the error for in non-root test runs.

Git-Dch: Ignore

20 files changed, 52 insertions, 59 deletions

diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc
index 6cf23daae..834776404 100644
--- a/apt-pkg/acquire-item.cc
+++ b/apt-pkg/acquire-item.cc
@@ -147,20 +147,6 @@ static bool BootstrapPDiffWith(std::string const &PartialFile, std::string const
    return typeItr != types.cend();
 }
 									/*}}}*/
-static bool RemoveFile(char const * const Function, std::string const &FileName)/*{{{*/
-{
-   if (FileName == "/dev/null")
-      return true;
-   errno = 0;
-   if (unlink(FileName.c_str()) != 0)
-   {
-      if (errno == ENOENT)
-	 return true;
-      return _error->WarningE(Function, "Removal of file %s failed", FileName.c_str());
-   }
-   return true;
-}
-									/*}}}*/
 
 static bool MessageInsecureRepository(bool const isError, std::string const &msg)/*{{{*/
 {
diff --git a/apt-pkg/acquire-worker.cc b/apt-pkg/acquire-worker.cc
index d8bdf5699..b5f52a3ca 100644
--- a/apt-pkg/acquire-worker.cc
+++ b/apt-pkg/acquire-worker.cc
@@ -742,9 +742,9 @@ void pkgAcquire::Worker::PrepareFiles(char const * const caller, pkgAcquire::Que
       for (pkgAcquire::Queue::QItem::owner_iterator O = Itm->Owners.begin(); O != Itm->Owners.end(); ++O)
       {
 	 pkgAcquire::Item const * const Owner = *O;
-	 if (Owner->DestFile == filename)
+	 if (Owner->DestFile == filename || filename == "/dev/null")
 	    continue;
-	 unlink(Owner->DestFile.c_str());
+	 RemoveFile("PrepareFiles", Owner->DestFile);
 	 if (link(filename.c_str(), Owner->DestFile.c_str()) != 0)
 	 {
 	    // different mounts can't happen for us as we download to lists/ by default,
@@ -759,7 +759,7 @@ void pkgAcquire::Worker::PrepareFiles(char const * const caller, pkgAcquire::Que
    else
    {
       for (pkgAcquire::Queue::QItem::owner_iterator O = Itm->Owners.begin(); O != Itm->Owners.end(); ++O)
-	 unlink((*O)->DestFile.c_str());
+	 RemoveFile("PrepareFiles", (*O)->DestFile);
    }
 }
 									/*}}}*/
diff --git a/apt-pkg/acquire.cc b/apt-pkg/acquire.cc
index a74f1c2f6..81faee5d8 100644
--- a/apt-pkg/acquire.cc
+++ b/apt-pkg/acquire.cc
@@ -645,7 +645,7 @@ bool pkgAcquire::Clean(string Dir)
       
       // Nothing found, nuke it
       if (I == Items.end())
-	 unlink(Dir->d_name);
+	 RemoveFile("Clean", Dir->d_name);
    };
    
    closedir(D);
@@ -993,15 +993,15 @@ void pkgAcquire::Queue::QItem::SyncDestinationFiles() const		/*{{{*/
       if (lstat((*O)->DestFile.c_str(),&file) == 0)
       {
 	 if ((file.st_mode & S_IFREG) == 0)
-	    unlink((*O)->DestFile.c_str());
+	    RemoveFile("SyncDestinationFiles", (*O)->DestFile);
 	 else if (supersize < file.st_size)
 	 {
 	    supersize = file.st_size;
-	    unlink(superfile.c_str());
+	    RemoveFile("SyncDestinationFiles", superfile);
 	    rename((*O)->DestFile.c_str(), superfile.c_str());
 	 }
 	 else
-	    unlink((*O)->DestFile.c_str());
+	    RemoveFile("SyncDestinationFiles", (*O)->DestFile);
 	 if (symlink(superfile.c_str(), (*O)->DestFile.c_str()) != 0)
 	 {
 	    ; // not a problem per-se and no real alternative
diff --git a/apt-pkg/cachefile.cc b/apt-pkg/cachefile.cc
index 1c9bc694b..aaa2436c5 100644
--- a/apt-pkg/cachefile.cc
+++ b/apt-pkg/cachefile.cc
@@ -191,9 +191,9 @@ void pkgCacheFile::RemoveCaches()
    std::string const srcpkgcache = _config->FindFile("Dir::cache::srcpkgcache");
 
    if (pkgcache.empty() == false && RealFileExists(pkgcache) == true)
-      unlink(pkgcache.c_str());
+      RemoveFile("RemoveCaches", pkgcache);
    if (srcpkgcache.empty() == false && RealFileExists(srcpkgcache) == true)
-      unlink(srcpkgcache.c_str());
+      RemoveFile("RemoveCaches", srcpkgcache);
    if (pkgcache.empty() == false)
    {
       std::string cachedir = flNotFile(pkgcache);
@@ -207,7 +207,7 @@ void pkgCacheFile::RemoveCaches()
 	    std::string nuke = flNotDir(*file);
 	    if (strncmp(cachefile.c_str(), nuke.c_str(), cachefile.length()) != 0)
 	       continue;
-	    unlink(file->c_str());
+	    RemoveFile("RemoveCaches", *file);
 	 }
       }
    }
@@ -226,7 +226,7 @@ void pkgCacheFile::RemoveCaches()
       std::string nuke = flNotDir(*file);
       if (strncmp(cachefile.c_str(), nuke.c_str(), cachefile.length()) != 0)
 	 continue;
-      unlink(file->c_str());
+      RemoveFile("RemoveCaches", *file);
    }
 }
 									/*}}}*/
diff --git a/apt-pkg/cdrom.cc b/apt-pkg/cdrom.cc
index b950648b9..29eeca066 100644
--- a/apt-pkg/cdrom.cc
+++ b/apt-pkg/cdrom.cc
@@ -426,8 +426,8 @@ bool pkgCdrom::WriteDatabase(Configuration &Cnf)
 {
    string DFile = _config->FindFile("Dir::State::cdroms");
    string NewFile = DFile + ".new";
-   
-   unlink(NewFile.c_str());
+
+   RemoveFile("WriteDatabase", NewFile);
    ofstream Out(NewFile.c_str());
    if (!Out)
       return _error->Errno("ofstream::ofstream",
@@ -468,7 +468,7 @@ bool pkgCdrom::WriteSourceList(string Name,vector<string> &List,bool Source)
       return _error->Errno("ifstream::ifstream","Opening %s",File.c_str());
 
    string NewFile = File + ".new";
-   unlink(NewFile.c_str());
+   RemoveFile("WriteDatabase", NewFile);
    ofstream Out(NewFile.c_str());
    if (!Out)
       return _error->Errno("ofstream::ofstream",
diff --git a/apt-pkg/contrib/fileutl.cc b/apt-pkg/contrib/fileutl.cc
index 368380af7..e52c8f219 100644
--- a/apt-pkg/contrib/fileutl.cc
+++ b/apt-pkg/contrib/fileutl.cc
@@ -172,6 +172,21 @@ bool CopyFile(FileFd &From,FileFd &To)
    return true;
 }
 									/*}}}*/
+bool RemoveFile(char const * const Function, std::string const &FileName)/*{{{*/
+{
+   if (FileName == "/dev/null")
+      return true;
+   errno = 0;
+   if (unlink(FileName.c_str()) != 0)
+   {
+      if (errno == ENOENT)
+	 return true;
+
+      return _error->WarningE(Function,_("Problem unlinking the file %s"), FileName.c_str());
+   }
+   return true;
+}
+									/*}}}*/
 // GetLock - Gets a lock file						/*{{{*/
 // ---------------------------------------------------------------------
 /* This will create an empty file of the given name and lock it. Once this
@@ -1135,13 +1150,13 @@ bool FileFd::Open(string FileName,unsigned int const Mode,APT::Configuration::Co
    else if ((OpenMode & (Exclusive | Create)) == (Exclusive | Create))
    {
       // for atomic, this will be done by rename in Close()
-      unlink(FileName.c_str());
+      RemoveFile("FileFd::Open", FileName);
    }
    if ((OpenMode & Empty) == Empty)
    {
       struct stat Buf;
       if (lstat(FileName.c_str(),&Buf) == 0 && S_ISLNK(Buf.st_mode))
-	 unlink(FileName.c_str());
+	 RemoveFile("FileFd::Open", FileName);
    }
 
    int fileflags = 0;
@@ -2025,8 +2040,7 @@ bool FileFd::Close()
 
    if ((Flags & Fail) == Fail && (Flags & DelOnFail) == DelOnFail &&
        FileName.empty() == false)
-      if (unlink(FileName.c_str()) != 0)
-	 Res &= _error->WarningE("unlnk",_("Problem unlinking the file %s"), FileName.c_str());
+      Res &= RemoveFile("FileFd::Close", FileName);
 
    if (Res == false)
       Flags |= Fail;
diff --git a/apt-pkg/contrib/fileutl.h b/apt-pkg/contrib/fileutl.h
index cddfe2b45..7176e4dea 100644
--- a/apt-pkg/contrib/fileutl.h
+++ b/apt-pkg/contrib/fileutl.h
@@ -150,6 +150,7 @@ class FileFd
 
 bool RunScripts(const char *Cnf);
 bool CopyFile(FileFd &From,FileFd &To);
+bool RemoveFile(char const * const Function, std::string const &FileName);
 int GetLock(std::string File,bool Errors = true);
 bool FileExists(std::string File);
 bool RealFileExists(std::string File);
diff --git a/apt-pkg/deb/dpkgpm.cc b/apt-pkg/deb/dpkgpm.cc
index ccc4b5a6c..1446826d6 100644
--- a/apt-pkg/deb/dpkgpm.cc
+++ b/apt-pkg/deb/dpkgpm.cc
@@ -1597,7 +1597,7 @@ bool pkgDPkgPM::Go(APT::Progress::PackageManager *progress)
    {
       std::string const oldpkgcache = _config->FindFile("Dir::cache::pkgcache");
       if (oldpkgcache.empty() == false && RealFileExists(oldpkgcache) == true &&
-	  unlink(oldpkgcache.c_str()) == 0)
+	  RemoveFile("pkgDPkgPM::Go", oldpkgcache))
       {
 	 std::string const srcpkgcache = _config->FindFile("Dir::cache::srcpkgcache");
 	 if (srcpkgcache.empty() == false && RealFileExists(srcpkgcache) == true)
diff --git a/apt-pkg/edsp/edsplistparser.cc b/apt-pkg/edsp/edsplistparser.cc
index 77a0edc22..85a57479e 100644
--- a/apt-pkg/edsp/edsplistparser.cc
+++ b/apt-pkg/edsp/edsplistparser.cc
@@ -31,12 +31,10 @@ public:
    edspListParserPrivate()
    {
       std::string const states = _config->FindFile("Dir::State::extended_states");
-      if (states != "/dev/null")
-	 unlink(states.c_str());
+      RemoveFile("edspListParserPrivate", states);
       extendedstates.Open(states, FileFd::WriteOnly | FileFd::Create | FileFd::Exclusive, 0600);
       std::string const prefs = _config->FindFile("Dir::Etc::preferences");
-      if (prefs != "/dev/null")
-	 unlink(prefs.c_str());
+      RemoveFile("edspListParserPrivate", prefs);
       preferences.Open(prefs, FileFd::WriteOnly | FileFd::Create | FileFd::Exclusive, 0600);
    }
 };
diff --git a/apt-pkg/edsp/edspsystem.cc b/apt-pkg/edsp/edspsystem.cc
index 4c16f76d2..95abc1527 100644
--- a/apt-pkg/edsp/edspsystem.cc
+++ b/apt-pkg/edsp/edspsystem.cc
@@ -57,8 +57,8 @@ public:
       if (tempDir.empty())
 	 return;
 
-      unlink(tempStatesFile.c_str());
-      unlink(tempPrefsFile.c_str());
+      RemoveFile("DeInitialize", tempStatesFile);
+      RemoveFile("DeInitialize", tempPrefsFile);
       rmdir(tempDir.c_str());
    }
 
diff --git a/apt-private/private-download.cc b/apt-private/private-download.cc
index 4894c72bf..dcb604f2a 100644
--- a/apt-private/private-download.cc
+++ b/apt-private/private-download.cc
@@ -331,7 +331,7 @@ bool DoClean(CommandLine &)
 	 c1out << "Del " << Pkg << " " << Ver << " [" << SizeToStr(St.st_size) << "B]" << std::endl;
 
 	 if (_config->FindB("APT::Get::Simulate") == false)
-	    unlink(File);
+	    RemoveFile("Cleaner::Erase", File);
       };
 };
 bool DoAutoClean(CommandLine &)
diff --git a/cmdline/apt-dump-solver.cc b/cmdline/apt-dump-solver.cc
index 47b515be5..27592f22e 100644
--- a/cmdline/apt-dump-solver.cc
+++ b/cmdline/apt-dump-solver.cc
@@ -53,8 +53,7 @@ int main(int argc,const char *argv[])					/*{{{*/
 	   return 0;
 	}
 
-	if (strcmp(filename, "/dev/null") != 0)
-	   unlink(filename);
+	RemoveFile(argv[0], filename);
 	FileFd input, output;
 	if (input.OpenDescriptor(STDIN_FILENO, FileFd::ReadOnly) == false ||
 	      output.Open(filename, FileFd::WriteOnly | FileFd::Create | FileFd::Exclusive, 0600) == false ||
diff --git a/ftparchive/byhash.cc b/ftparchive/byhash.cc
index 0a38457c0..354d089c3 100644
--- a/ftparchive/byhash.cc
+++ b/ftparchive/byhash.cc
@@ -41,9 +41,8 @@ void DeleteAllButMostRecent(std::string dir, int KeepFiles)
    auto files = GetListOfFilesInDir(dir, false);
    std::sort(files.begin(), files.end(), Cmp());
 
-   for (auto I=files.begin(); I<files.end()-KeepFiles; I++) {
-      unlink((*I).c_str());
-   }
+   for (auto I=files.begin(); I<files.end()-KeepFiles; ++I)
+      RemoveFile("DeleteAllButMostRecent", *I);
 }
 
 // Takes a input filename (e.g. binary-i386/Packages) and a hashstring
diff --git a/ftparchive/multicompress.cc b/ftparchive/multicompress.cc
index 08a3cff5a..3ffc5266e 100644
--- a/ftparchive/multicompress.cc
+++ b/ftparchive/multicompress.cc
@@ -352,9 +352,7 @@ bool MultiCompress::Child(int const &FD)
 	 for (Files *I = Outputs; I != 0; I = I->Next)
 	 {
 	    I->TmpFile.Close();
-	    if (unlink(I->TmpFile.Name().c_str()) != 0)
-	       _error->Errno("unlink",_("Problem unlinking %s"),
-			     I->TmpFile.Name().c_str());
+	    RemoveFile("MultiCompress::Child", I->TmpFile.Name());
 	 }
 	 return !_error->PendingError();
       }      
diff --git a/ftparchive/writer.cc b/ftparchive/writer.cc
index 0bd2be566..c0223a74c 100644
--- a/ftparchive/writer.cc
+++ b/ftparchive/writer.cc
@@ -302,9 +302,7 @@ bool FTWScanner::Delink(string &FileName,const char *OriginalPath,
 	       _error->Errno("readlink",_("Failed to readlink %s"),OriginalPath);
 	    else
 	    {
-	       if (unlink(OriginalPath) != 0)
-		  _error->Errno("unlink",_("Failed to unlink %s"),OriginalPath);
-	       else
+	       if (RemoveFile("FTWScanner::Delink", OriginalPath))
 	       {
 		  if (link(FileName.c_str(),OriginalPath) != 0)
 		  {
diff --git a/methods/file.cc b/methods/file.cc
index b689de619..8a087c36d 100644
--- a/methods/file.cc
+++ b/methods/file.cc
@@ -76,7 +76,7 @@ bool FileMethod::Fetch(FetchItem *Itm)
       }
    }
    if (Res.IMSHit != true)
-      unlink(Itm->DestFile.c_str());
+      RemoveFile("file", Itm->DestFile);
 
    // See if the file exists
    if (stat(File.c_str(),&Buf) == 0)
diff --git a/methods/ftp.cc b/methods/ftp.cc
index d84a194ca..360333107 100644
--- a/methods/ftp.cc
+++ b/methods/ftp.cc
@@ -1090,7 +1090,7 @@ bool FtpMethod::Fetch(FetchItem *Itm)
 	 // If the file is missing we hard fail and delete the destfile
 	 // otherwise transient fail
 	 if (Missing == true) {
-	    unlink(FailFile.c_str());
+	    RemoveFile("ftp", FailFile);
 	    return false;
 	 }
 	 Fail(true);
diff --git a/methods/https.cc b/methods/https.cc
index a15915910..8d9454545 100644
--- a/methods/https.cc
+++ b/methods/https.cc
@@ -443,7 +443,7 @@ bool HttpsMethod::Fetch(FetchItem *Itm)
    // server says file not modified
    if (Server->Result == 304 || curl_condition_unmet == 1)
    {
-      unlink(File->Name().c_str());
+      RemoveFile("https", File->Name());
       Res.IMSHit = true;
       Res.LastModified = Itm->LastModified;
       Res.Size = 0;
@@ -461,14 +461,14 @@ bool HttpsMethod::Fetch(FetchItem *Itm)
       SetFailReason(err);
       _error->Error("%i %s", Server->Result, Server->Code);
       // unlink, no need keep 401/404 page content in partial/
-      unlink(File->Name().c_str());
+      RemoveFile("https", File->Name());
       return false;
    }
 
    // invalid range-request
    if (Server->Result == 416)
    {
-      unlink(File->Name().c_str());
+      RemoveFile("https", File->Name());
       delete File;
       Redirect(Itm->Uri);
       return true;
diff --git a/methods/mirror.cc b/methods/mirror.cc
index d3aef91bc..56362d317 100644
--- a/methods/mirror.cc
+++ b/methods/mirror.cc
@@ -122,7 +122,7 @@ bool MirrorMethod::Clean(string Dir)
       }
       // nothing found, nuke it
       if (I == list.end())
-	 unlink(Dir->d_name);
+	 RemoveFile("mirror", Dir->d_name);
    }
 
    closedir(D);
diff --git a/methods/server.cc b/methods/server.cc
index 934ec2abe..650a4aeb8 100644
--- a/methods/server.cc
+++ b/methods/server.cc
@@ -274,7 +274,7 @@ ServerMethod::DealWithHeaders(FetchResult &Res)
    // Not Modified
    if (Server->Result == 304)
    {
-      unlink(Queue->DestFile.c_str());
+      RemoveFile("server", Queue->DestFile);
       Res.IMSHit = true;
       Res.LastModified = Queue->LastModified;
       return IMS_HIT;
@@ -350,7 +350,7 @@ ServerMethod::DealWithHeaders(FetchResult &Res)
 	    Server->StartPos = Server->TotalFileSize;
 	    Server->Result = 200;
 	 }
-	 else if (unlink(Queue->DestFile.c_str()) == 0)
+	 else if (RemoveFile("server", Queue->DestFile))
 	 {
 	    NextURI = Queue->Uri;
 	    return TRY_AGAIN_OR_REDIRECT;