diff -Nru ubuntu-download-manager-0.3+14.04.20140305/debian/changelog ubuntu-download-manager-0.3+14.04.20140317/debian/changelog --- ubuntu-download-manager-0.3+14.04.20140305/debian/changelog 2014-03-17 17:29:13.000000000 +0000 +++ ubuntu-download-manager-0.3+14.04.20140317/debian/changelog 2014-03-17 17:29:13.000000000 +0000 @@ -1,3 +1,14 @@ +ubuntu-download-manager (0.3+14.04.20140317-0ubuntu1) trusty; urgency=low + + [ Manuel de la Peña ] + * Fix some small issues with the logging and ensure that the mutex + paht is the correct one when working with a not confined app that + passed a local path. (LP: #1287245) + * Ensure that Error and Warning are written in the correct dir. (LP: + #1288727) + + -- Ubuntu daily release Mon, 17 Mar 2014 17:25:03 +0000 + ubuntu-download-manager (0.3+14.04.20140305-0ubuntu1) trusty; urgency=low * New rebuild forced diff -Nru ubuntu-download-manager-0.3+14.04.20140305/ubuntu-download-manager-priv/downloads/file_download.cpp ubuntu-download-manager-0.3+14.04.20140317/ubuntu-download-manager-priv/downloads/file_download.cpp --- ubuntu-download-manager-0.3+14.04.20140305/ubuntu-download-manager-priv/downloads/file_download.cpp 2014-03-05 13:16:21.000000000 +0000 +++ ubuntu-download-manager-0.3+14.04.20140317/ubuntu-download-manager-priv/downloads/file_download.cpp 2014-03-17 17:24:45.000000000 +0000 @@ -30,7 +30,7 @@ #include "system/logger.h" #include "system/network_reply.h" -#define DOWN_LOG(LEVEL) LOG(LEVEL) << ((parent() != nullptr)?"GroupDownload{" + parent()->objectName() + "} ":"") << "Download ID{" << objectName() << "}" +#define DOWN_LOG(LEVEL) LOG(LEVEL) << ((parent() != nullptr)?"GroupDownload {" + parent()->objectName() + " } ":"") << "Download ID{" << objectName() << " } " namespace { @@ -117,7 +117,7 @@ cleanUpCurrentData(); // unlock the file name so that other downloads can use it it if is // no used in the file system - _fileNameMutex->unlockFileName(_filePath); + unlockFilePath(); _downloading = false; emit canceled(true); } @@ -623,19 +623,34 @@ FileDownload::emitFinished() { auto fileMan = FileManager::instance(); - LOG(INFO) << "EMIT finished" << filePath(); - setState(Download::FINISH); - if (fileMan->exists(_tempFilePath)) { - LOG(INFO) << "Rename '" << _tempFilePath << "' to '" + DOWN_LOG(INFO) << "Rename '" << _tempFilePath << "' to '" << _filePath << "'"; - fileMan->rename(_tempFilePath, _filePath); + QFile tempFile(_tempFilePath); + auto r = tempFile.rename(_filePath); + if (!r) { + DOWN_LOG(WARNING) << "Could not rename '" << _tempFilePath << "' to '" + << _filePath << "' due to " << tempFile.errorString(); + } } - _fileNameMutex->unlockFileName(_filePath); + + setState(Download::FINISH); + unlockFilePath(); + + DOWN_LOG(INFO) << "EMIT finished" << filePath(); emit finished(_filePath); } void +FileDownload::unlockFilePath() { + if (!isConfined() && metadata().contains(Metadata::LOCAL_PATH_KEY)) { + _fileNameMutex->unlockFileName(_tempFilePath); + } else { + _fileNameMutex->unlockFileName(_filePath); + } +} + +void FileDownload::cleanUpCurrentData() { bool success = true; QFile::FileError error = QFile::NoError; @@ -685,7 +700,7 @@ _reply = nullptr; cleanUpCurrentData(); // let other downloads use the same file name - _fileNameMutex->unlockFileName(_filePath); + unlockFilePath(); Download::emitError(error); } diff -Nru ubuntu-download-manager-0.3+14.04.20140305/ubuntu-download-manager-priv/downloads/file_download.h ubuntu-download-manager-0.3+14.04.20140317/ubuntu-download-manager-priv/downloads/file_download.h --- ubuntu-download-manager-0.3+14.04.20140305/ubuntu-download-manager-priv/downloads/file_download.h 2014-03-05 13:16:21.000000000 +0000 +++ ubuntu-download-manager-0.3+14.04.20140317/ubuntu-download-manager-priv/downloads/file_download.h 2014-03-17 17:24:45.000000000 +0000 @@ -122,6 +122,7 @@ void onOnlineStateChanged(bool); void initFileNames(); void emitFinished(); + void unlockFilePath(); private: bool _downloading = false; diff -Nru ubuntu-download-manager-0.3+14.04.20140305/ubuntu-download-manager-priv/downloads/group_download.cpp ubuntu-download-manager-0.3+14.04.20140317/ubuntu-download-manager-priv/downloads/group_download.cpp --- ubuntu-download-manager-0.3+14.04.20140305/ubuntu-download-manager-priv/downloads/group_download.cpp 2014-03-05 13:16:21.000000000 +0000 +++ ubuntu-download-manager-0.3+14.04.20140317/ubuntu-download-manager-priv/downloads/group_download.cpp 2014-03-17 17:24:45.000000000 +0000 @@ -16,6 +16,8 @@ * boston, ma 02110-1301, usa. */ +#include +#include #include #include #include "downloads/download_adaptor.h" @@ -24,7 +26,7 @@ #include "system/logger.h" #include "system/uuid_factory.h" -#define GROUP_LOG(LEVEL) LOG(LEVEL) << "Group Download{" << objectName() << "}" +#define GROUP_LOG(LEVEL) LOG(LEVEL) << "Group Download {" << objectName() << " } " namespace Ubuntu { @@ -167,6 +169,7 @@ GroupDownload::cancelDownload() { TRACE; cancelAllDownloads(); + GROUP_LOG(INFO) << "EMIT canceled"; emit canceled(true); } @@ -181,6 +184,7 @@ download->pauseDownload(); } } + GROUP_LOG(INFO) << "EMIT paused"; emit paused(true); } @@ -193,6 +197,7 @@ download->resumeDownload(); } } + GROUP_LOG(INFO) << "EMIT resumed"; emit resumed(true); } @@ -206,9 +211,12 @@ download->startDownload(); } } + GROUP_LOG(INFO) << "EMIT started"; emit started(true); } else { + GROUP_LOG(INFO) << "EMIT started"; emit started(true); + GROUP_LOG(INFO) << "EMIT finished " << _finishedDownloads.join(";"); emit finished(_finishedDownloads); } } @@ -257,6 +265,8 @@ // files that we managed to download cancelAllDownloads(); QString errorMsg = down->url().toString() + ":" + error; + + GROUP_LOG(INFO) << "EMIT error " << errorMsg; emitError(errorMsg); } @@ -351,6 +361,17 @@ // that downloads we are done :) if (_downloads.count() == _finishedDownloads.count()) { setState(Download::FINISH); +#ifndef NDEBUG + foreach(const QString& file, _finishedDownloads) { + auto parentDir = QFileInfo(file).dir(); + DLOG(INFO) << "Files for path dir '" << parentDir.absolutePath() << "' :"; + auto children = parentDir.entryList(); + foreach(const QString& child, children) { + DLOG(INFO) << "\t" << child; + } + } +#endif + GROUP_LOG(INFO) << "EMIT finished " << _finishedDownloads.join(";"); emit finished(_finishedDownloads); } } diff -Nru ubuntu-download-manager-0.3+14.04.20140305/ubuntu-download-manager-priv/system/filename_mutex.cpp ubuntu-download-manager-0.3+14.04.20140317/ubuntu-download-manager-priv/system/filename_mutex.cpp --- ubuntu-download-manager-0.3+14.04.20140305/ubuntu-download-manager-priv/system/filename_mutex.cpp 2014-03-05 13:16:21.000000000 +0000 +++ ubuntu-download-manager-0.3+14.04.20140317/ubuntu-download-manager-priv/system/filename_mutex.cpp 2014-03-17 17:24:45.000000000 +0000 @@ -82,7 +82,7 @@ _mutex.lock(); auto removed = _paths.remove(filename); if (!removed) { - LOG(WARNING) << "Tired to remove filename '" << filename + LOG(WARNING) << "Tried to remove filename '" << filename << "' when it was not owned by any object."; } else { LOG(INFO) << "Released path '" << filename << "'"; diff -Nru ubuntu-download-manager-0.3+14.04.20140305/ubuntu-download-manager-priv/system/logger.cpp ubuntu-download-manager-0.3+14.04.20140317/ubuntu-download-manager-priv/system/logger.cpp --- ubuntu-download-manager-0.3+14.04.20140305/ubuntu-download-manager-priv/system/logger.cpp 2014-03-05 13:16:21.000000000 +0000 +++ ubuntu-download-manager-0.3+14.04.20140317/ubuntu-download-manager-priv/system/logger.cpp 2014-03-17 17:24:55.000000000 +0000 @@ -125,6 +125,8 @@ if (!_init) { _init = true; google::InitGoogleLogging(toStdString(appName).c_str()); + google::SetLogDestination(google::ERROR, toStdString(path).c_str()); + google::SetLogDestination(google::WARNING, toStdString(path).c_str()); google::SetLogDestination(google::INFO, toStdString(path).c_str()); } } diff -Nru ubuntu-download-manager-0.3+14.04.20140305/ubuntu-download-manager-tests/downloads/test_download.cpp ubuntu-download-manager-0.3+14.04.20140317/ubuntu-download-manager-tests/downloads/test_download.cpp --- ubuntu-download-manager-0.3+14.04.20140305/ubuntu-download-manager-tests/downloads/test_download.cpp 2014-03-05 13:16:21.000000000 +0000 +++ ubuntu-download-manager-0.3+14.04.20140317/ubuntu-download-manager-tests/downloads/test_download.cpp 2014-03-17 17:24:45.000000000 +0000 @@ -2285,8 +2285,14 @@ QCOMPARE(2, calledMethods.count()); auto methodName = calledMethods[0].methodName(); QCOMPARE(QString("lockFileName"), methodName); + auto path = qobject_cast( + calledMethods[0].params().inParams()[0])->value(); + QCOMPARE(download->filePath(), path); methodName = calledMethods[1].methodName(); QCOMPARE(QString("unlockFileName"), methodName); + path = qobject_cast( + calledMethods[1].params().inParams()[0])->value(); + QCOMPARE(download->filePath(), path); } void @@ -2307,8 +2313,14 @@ QCOMPARE(2, calledMethods.count()); auto methodName = calledMethods[0].methodName(); QCOMPARE(QString("lockFileName"), methodName); + auto path = qobject_cast( + calledMethods[0].params().inParams()[0])->value(); + QCOMPARE(download->filePath(), path); methodName = calledMethods[1].methodName(); QCOMPARE(QString("unlockFileName"), methodName); + path = qobject_cast( + calledMethods[1].params().inParams()[0])->value(); + QCOMPARE(download->filePath(), path); } void @@ -2336,8 +2348,14 @@ QCOMPARE(2, calledMethods.count()); auto methodName = calledMethods[0].methodName(); QCOMPARE(QString("lockFileName"), methodName); + auto path = qobject_cast( + calledMethods[0].params().inParams()[0])->value(); + QCOMPARE(download->filePath(), path); methodName = calledMethods[1].methodName(); QCOMPARE(QString("unlockFileName"), methodName); + path = qobject_cast( + calledMethods[1].params().inParams()[0])->value(); + QCOMPARE(download->filePath(), path); } void @@ -2378,8 +2396,14 @@ QCOMPARE(2, calledMethods.count()); auto methodName = calledMethods[0].methodName(); QCOMPARE(QString("lockFileName"), methodName); + auto path = qobject_cast( + calledMethods[0].params().inParams()[0])->value(); + QCOMPARE(download->filePath(), path); methodName = calledMethods[1].methodName(); QCOMPARE(QString("unlockFileName"), methodName); + path = qobject_cast( + calledMethods[1].params().inParams()[0])->value(); + QCOMPARE(download->filePath(), path); } void @@ -2412,6 +2436,177 @@ QCOMPARE(2, calledMethods.count()); auto methodName = calledMethods[0].methodName(); QCOMPARE(QString("lockFileName"), methodName); + auto path = qobject_cast( + calledMethods[0].params().inParams()[0])->value(); + QCOMPARE(download->filePath(), path); methodName = calledMethods[1].methodName(); QCOMPARE(QString("unlockFileName"), methodName); + path = qobject_cast( + calledMethods[1].params().inParams()[0])->value(); + QCOMPARE(download->filePath(), path); +} + +void +TestDownload::testCancelUnlocksPathFromMetadata() { + QVariantMap metadata; + QString localPath = "/home/my/local/path"; + metadata["local-path"] = localPath; + + _fileNameMutex->record(); + QScopedPointer download(new FileDownload(_id, _path, + _isConfined, _rootPath, _url, metadata, _headers)); + QSignalSpy spy(download.data(), + SIGNAL(canceled(bool))); // NOLINT(readability/function) + + download->start(); // change state + download->startDownload(); + download->cancel(); // change state + download->cancelDownload(); // method under test + + // assert that the filename was correctly managed + auto calledMethods = _fileNameMutex->calledMethods(); + QCOMPARE(2, calledMethods.count()); + auto methodName = calledMethods[0].methodName(); + QCOMPARE(QString("lockFileName"), methodName); + auto path = qobject_cast( + calledMethods[0].params().inParams()[0])->value(); + QCOMPARE(download->filePath() + ".tmp", path); + methodName = calledMethods[1].methodName(); + QCOMPARE(QString("unlockFileName"), methodName); + path = qobject_cast( + calledMethods[1].params().inParams()[0])->value(); + QCOMPARE(download->filePath() + ".tmp", path); +} + +void +TestDownload::testFinishUnlocksPathFromMetadata() { + QVariantMap metadata; + QString localPath = "/home/my/local/path"; + metadata["local-path"] = localPath; + + _fileNameMutex->record(); + _reqFactory->record(); + QScopedPointer download(new FileDownload(_id, _path, + _isConfined, _rootPath, _url, metadata, _headers)); + QSignalSpy spy(download.data(), SIGNAL(finished(QString))); + + download->start(); // change state + download->startDownload(); + + QList calledMethods = _reqFactory->calledMethods(); + QCOMPARE(1, calledMethods.count()); + FakeNetworkReply* reply = reinterpret_cast( + calledMethods[0].params().outParams()[0]); + + // emit the finish signal and expect it to be raised + emit reply->finished(); + QCOMPARE(spy.count(), 1); + QCOMPARE(download->state(), Download::FINISH); + + calledMethods = _fileNameMutex->calledMethods(); + QCOMPARE(2, calledMethods.count()); + auto methodName = calledMethods[0].methodName(); + QCOMPARE(QString("lockFileName"), methodName); + auto path = qobject_cast( + calledMethods[0].params().inParams()[0])->value(); + QCOMPARE(download->filePath() + ".tmp", path); + methodName = calledMethods[1].methodName(); + QCOMPARE(QString("unlockFileName"), methodName); + path = qobject_cast( + calledMethods[1].params().inParams()[0])->value(); + QCOMPARE(download->filePath() + ".tmp", path); +} + +void +TestDownload::testProcessFinishUnlocksPathFromMetadata() { + _fileNameMutex->record(); + _processFactory->record(); + _reqFactory->record(); + + QString command = "cd"; + QVariantMap metadata; + metadata["post-download-command"] = command; + QString localPath = "/home/my/local/path"; + metadata["local-path"] = localPath; + + QScopedPointer download(new FileDownload(_id, _path, + _isConfined, _rootPath, _url, metadata, _headers)); + QSignalSpy spy(download.data(), SIGNAL(finished(QString))); + + download->start(); // change state + download->startDownload(); + + // we need to set the data before we pause!!! + QList calledMethods = _reqFactory->calledMethods(); + QCOMPARE(1, calledMethods.count()); + FakeNetworkReply* reply = reinterpret_cast( + calledMethods[0].params().outParams()[0]); + + // makes the process to be executed + reply->emitFinished(); + + calledMethods = _processFactory->calledMethods(); + QCOMPARE(1, calledMethods.count()); + FakeProcess* process = reinterpret_cast( + calledMethods[0].params().outParams()[0]); + + process->emitFinished(0, QProcess::NormalExit); + QTRY_COMPARE(spy.count(), 1); + + calledMethods = _fileNameMutex->calledMethods(); + QCOMPARE(2, calledMethods.count()); + auto methodName = calledMethods[0].methodName(); + QCOMPARE(QString("lockFileName"), methodName); + auto path = qobject_cast( + calledMethods[0].params().inParams()[0])->value(); + QCOMPARE(download->filePath() + ".tmp", path); + methodName = calledMethods[1].methodName(); + QCOMPARE(QString("unlockFileName"), methodName); + path = qobject_cast( + calledMethods[1].params().inParams()[0])->value(); + QCOMPARE(download->filePath() + ".tmp" , path); +} + +void +TestDownload::testErrorUnlocksPathFromMetadata() { + QVariantMap metadata; + QString localPath = "/home/my/local/path"; + metadata["local-path"] = localPath; + + // fake an error and make sure that unlock is called + _fileNameMutex->record(); + _reqFactory->record(); + QScopedPointer download(new FileDownload(_id, _path, + _isConfined, _rootPath, _url, metadata, _headers)); + QSignalSpy errorSpy(download.data(), SIGNAL(error(QString))); + + download->start(); // change state + download->startDownload(); + + // we need to set the data before we pause!!! + QList calledMethods = _reqFactory->calledMethods(); + QCOMPARE(1, calledMethods.count()); + FakeNetworkReply* reply = reinterpret_cast( + calledMethods[0].params().outParams()[0]); + + // set the attrs in the reply so that we do raise two signals + reply->setAttribute(QNetworkRequest::HttpStatusCodeAttribute, 404); + reply->setAttribute(QNetworkRequest::HttpReasonPhraseAttribute, ""); + + // emit the error and esure that the signals are raised + reply->emitHttpError(QNetworkReply::ContentAccessDenied); + QCOMPARE(errorSpy.count(), 1); + + calledMethods = _fileNameMutex->calledMethods(); + QCOMPARE(2, calledMethods.count()); + auto methodName = calledMethods[0].methodName(); + QCOMPARE(QString("lockFileName"), methodName); + auto path = qobject_cast( + calledMethods[0].params().inParams()[0])->value(); + QCOMPARE(download->filePath() + ".tmp", path); + methodName = calledMethods[1].methodName(); + QCOMPARE(QString("unlockFileName"), methodName); + path = qobject_cast( + calledMethods[1].params().inParams()[0])->value(); + QCOMPARE(download->filePath() + ".tmp", path); } diff -Nru ubuntu-download-manager-0.3+14.04.20140305/ubuntu-download-manager-tests/downloads/test_download.h ubuntu-download-manager-0.3+14.04.20140317/ubuntu-download-manager-tests/downloads/test_download.h --- ubuntu-download-manager-0.3+14.04.20140305/ubuntu-download-manager-tests/downloads/test_download.h 2014-03-05 13:16:21.000000000 +0000 +++ ubuntu-download-manager-0.3+14.04.20140317/ubuntu-download-manager-tests/downloads/test_download.h 2014-03-17 17:24:45.000000000 +0000 @@ -169,6 +169,10 @@ void testFinishUnlocksPath(); void testProcessFinishUnlocksPath(); void testErrorUnlocksPath(); + void testCancelUnlocksPathFromMetadata(); + void testFinishUnlocksPathFromMetadata(); + void testProcessFinishUnlocksPathFromMetadata(); + void testErrorUnlocksPathFromMetadata(); private: QString _id;