diff --git a/ChangeLog b/ChangeLog index fa1c50e0..bef852bb 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,15 @@ +2008-09-07 Tatsuhiro Tsujikawa + + Fixed the bug that DiskWriterEntry is not created when its + FileEntry.isRequested() is false and it doesn't share a piece with + other FileEntries that are requested. This bug causes segmentation fault + in the end. + * src/MultiDiskAdaptor.cc + * src/MultiDiskAdaptor.h + * src/MultiFileAllocationIterator.cc + * test/MultiDiskAdaptorTest.cc + * test/MultiFileAllocationIteratorTest.cc + 2008-09-07 Tatsuhiro Tsujikawa Fixed the bug that exception is thrown when MultiDiskAdaptor::size() is diff --git a/src/MultiDiskAdaptor.cc b/src/MultiDiskAdaptor.cc index 2df1b68d..a2377ace 100644 --- a/src/MultiDiskAdaptor.cc +++ b/src/MultiDiskAdaptor.cc @@ -45,11 +45,13 @@ #include "Logger.h" #include "SimpleRandomizer.h" #include +#include namespace aria2 { DiskWriterEntry::DiskWriterEntry(const SharedHandle& fileEntry): - fileEntry(fileEntry), _open(false), _directIO(false) {} + fileEntry(fileEntry), _open(false), _directIO(false), + _needsFileAllocation(false) {} DiskWriterEntry::~DiskWriterEntry() {} @@ -60,29 +62,35 @@ std::string DiskWriterEntry::getFilePath(const std::string& topDir) const void DiskWriterEntry::initAndOpenFile(const std::string& topDir) { - diskWriter->initAndOpenFile(getFilePath(topDir), fileEntry->getLength()); - if(_directIO) { - diskWriter->enableDirectIO(); + if(!diskWriter.isNull()) { + diskWriter->initAndOpenFile(getFilePath(topDir), fileEntry->getLength()); + if(_directIO) { + diskWriter->enableDirectIO(); + } + _open = true; } - _open = true; } void DiskWriterEntry::openFile(const std::string& topDir) { - diskWriter->openFile(getFilePath(topDir), fileEntry->getLength()); - if(_directIO) { - diskWriter->enableDirectIO(); + if(!diskWriter.isNull()) { + diskWriter->openFile(getFilePath(topDir), fileEntry->getLength()); + if(_directIO) { + diskWriter->enableDirectIO(); + } + _open = true; } - _open = true; } void DiskWriterEntry::openExistingFile(const std::string& topDir) { - diskWriter->openExistingFile(getFilePath(topDir), fileEntry->getLength()); - if(_directIO) { - diskWriter->enableDirectIO(); + if(!diskWriter.isNull()) { + diskWriter->openExistingFile(getFilePath(topDir), fileEntry->getLength()); + if(_directIO) { + diskWriter->enableDirectIO(); + } + _open = true; } - _open = true; } bool DiskWriterEntry::isOpen() const @@ -105,6 +113,7 @@ bool DiskWriterEntry::fileExists(const std::string& topDir) uint64_t DiskWriterEntry::size() const { + assert(!diskWriter.isNull()); return diskWriter->size(); } @@ -144,6 +153,16 @@ void DiskWriterEntry::disableDirectIO() _directIO = false; } +bool DiskWriterEntry::needsFileAllocation() const +{ + return _needsFileAllocation; +} + +void DiskWriterEntry::needsFileAllocation(bool f) +{ + _needsFileAllocation = f; +} + MultiDiskAdaptor::MultiDiskAdaptor(): pieceLength(0), _maxOpenFiles(DEFAULT_MAX_OPEN_FILES), @@ -153,13 +172,10 @@ MultiDiskAdaptor::~MultiDiskAdaptor() {} static SharedHandle createDiskWriterEntry (const SharedHandle& fileEntry, - DiskWriterFactory& dwFactory, - bool directIOAllowed) + bool needsFileAllocation) { SharedHandle entry(new DiskWriterEntry(fileEntry)); - entry->setDiskWriter(dwFactory.newDiskWriter()); - entry->getDiskWriter()->setDirectIOAllowed(directIOAllowed); - + entry->needsFileAllocation(needsFileAllocation); return entry; } @@ -172,49 +188,42 @@ void MultiDiskAdaptor::resetDiskWriterEntries() return; } - DefaultDiskWriterFactory dwFactory; - if(pieceLength == 0) { - for(std::deque >::const_iterator itr = - fileEntries.begin(); itr != fileEntries.end(); ++itr) { - if((*itr)->isRequested()) { - diskWriterEntries.push_back - (createDiskWriterEntry(*itr, dwFactory, _directIOAllowed)); - } - } - } else { - std::deque >::const_iterator done = fileEntries.begin(); - for(std::deque >::const_iterator itr = - fileEntries.begin(); itr != fileEntries.end();) { - if(!(*itr)->isRequested()) { + for(std::deque >::const_iterator i = + fileEntries.begin(); i != fileEntries.end(); ++i) { + diskWriterEntries.push_back + (createDiskWriterEntry(*i, (*i)->isRequested())); + } + + // TODO Currently, pieceLength == 0 is used for unit testing only. + if(pieceLength > 0) { + std::deque >::iterator done = + diskWriterEntries.begin(); + for(std::deque >::iterator itr = + diskWriterEntries.begin(); itr != diskWriterEntries.end();) { + if(!(*itr)->getFileEntry()->isRequested()) { ++itr; continue; } - off_t pieceStartOffset = ((*itr)->getOffset()/pieceLength)*pieceLength; - std::deque >::iterator insertionPoint = - diskWriterEntries.end(); - - if(itr != fileEntries.begin()) { - for(std::deque >::const_iterator i = itr-1; - i != done; --i) { - if((uint64_t)pieceStartOffset < (*i)->getOffset()+(*i)->getLength()) { - insertionPoint = diskWriterEntries.insert - (insertionPoint, - createDiskWriterEntry(*i, dwFactory, _directIOAllowed)); + off_t pieceStartOffset = + ((*itr)->getFileEntry()->getOffset()/pieceLength)*pieceLength; + if(itr != diskWriterEntries.begin()) { + for(std::deque >::iterator i = + itr-1; i != done; --i) { + const SharedHandle& fileEntry = (*i)->getFileEntry(); + if((uint64_t)pieceStartOffset < + fileEntry->getOffset()+fileEntry->getLength()) { + (*i)->needsFileAllocation(true); } else { break; } } } - diskWriterEntries.push_back - (createDiskWriterEntry(*itr, dwFactory, _directIOAllowed)); - ++itr; - for(; itr != fileEntries.end(); ++itr) { - if((*itr)->getOffset() < pieceStartOffset+pieceLength) { - diskWriterEntries.push_back - (createDiskWriterEntry(*itr, dwFactory, _directIOAllowed)); + for(; itr != diskWriterEntries.end(); ++itr) { + if((*itr)->getFileEntry()->getOffset() < pieceStartOffset+pieceLength) { + (*itr)->needsFileAllocation(true); } else { break; } @@ -223,6 +232,16 @@ void MultiDiskAdaptor::resetDiskWriterEntries() done = itr-1; } } + DefaultDiskWriterFactory dwFactory; + for(std::deque >::iterator i = + diskWriterEntries.begin(); i != diskWriterEntries.end(); ++i) { + if((*i)->needsFileAllocation() || (*i)->fileExists(getTopDirPath())) { + logger->debug("Creating DiskWriter for filename=%s", + (*i)->getFilePath(getTopDirPath()).c_str()); + (*i)->setDiskWriter(dwFactory.newDiskWriter()); + (*i)->getDiskWriter()->setDirectIOAllowed(_directIOAllowed); + } + } } std::string MultiDiskAdaptor::getTopDirPath() const diff --git a/src/MultiDiskAdaptor.h b/src/MultiDiskAdaptor.h index bc7500f2..4533b7ce 100644 --- a/src/MultiDiskAdaptor.h +++ b/src/MultiDiskAdaptor.h @@ -49,6 +49,7 @@ private: SharedHandle diskWriter; bool _open; bool _directIO; + bool _needsFileAllocation; public: DiskWriterEntry(const SharedHandle& fileEntry); @@ -87,6 +88,10 @@ public: // Additionally, if diskWriter is opened, diskWriter->disableDirectIO() is // called. void disableDirectIO(); + + bool needsFileAllocation() const; + + void needsFileAllocation(bool f); }; typedef SharedHandle DiskWriterEntryHandle; diff --git a/src/MultiFileAllocationIterator.cc b/src/MultiFileAllocationIterator.cc index 1f541361..7fcb7b3e 100644 --- a/src/MultiFileAllocationIterator.cc +++ b/src/MultiFileAllocationIterator.cc @@ -61,7 +61,7 @@ void MultiFileAllocationIterator::allocateChunk() _diskAdaptor->openIfNot(entry, &DiskWriterEntry::openFile, _diskAdaptor->getTopDirPath()); entry->enableDirectIO(); - if(entry->size() < fileEntry->getLength()) { + if(entry->needsFileAllocation() && entry->size() < fileEntry->getLength()) { // Calling private function of MultiDiskAdaptor. _fileAllocationIterator.reset (new SingleFileAllocationIterator(entry->getDiskWriter().get(), diff --git a/test/MultiDiskAdaptorTest.cc b/test/MultiDiskAdaptorTest.cc index 295bce55..0cc38dde 100644 --- a/test/MultiDiskAdaptorTest.cc +++ b/test/MultiDiskAdaptorTest.cc @@ -158,6 +158,7 @@ void MultiDiskAdaptorTest::testCutTrailingGarbage() adaptor.setTopDir(topDir); adaptor.setFileEntries(fileEntries); adaptor.setMaxOpenFiles(1); + adaptor.setPieceLength(1); adaptor.openFile(); @@ -191,6 +192,7 @@ void MultiDiskAdaptorTest::testSize() adaptor.setTopDir(topDir); adaptor.setFileEntries(fileEntries); adaptor.setMaxOpenFiles(1); + adaptor.setPieceLength(1); adaptor.openFile(); diff --git a/test/MultiFileAllocationIteratorTest.cc b/test/MultiFileAllocationIteratorTest.cc index 40c12646..288df5e2 100644 --- a/test/MultiFileAllocationIteratorTest.cc +++ b/test/MultiFileAllocationIteratorTest.cc @@ -4,6 +4,8 @@ #include "FileEntry.h" #include "Exception.h" #include "array_fun.h" +#include "TestUtil.h" +#include "DiskWriter.h" #include #include #include @@ -50,37 +52,93 @@ void MultiFileAllocationIteratorTest::testMakeDiskWriterEntries() fs[6]->setRequested(false); // file7 fs[8]->setRequested(false); // file9 fs[9]->setRequested(false); // fileA + + std::string topDir = "top"; + std::string storeDir = "/tmp/aria2_MultiFileAllocationIteratorTest" + "_testMakeDiskWriterEntries"; + std::string prefix = storeDir+"/"+topDir; + + // create empty file4 + createFile(prefix+std::string("/file4"), 0); - std::string storeDir = "/tmp/aria2_MultiFileAllocationIteratorTest_testMakeDiskWriterEntries"; SharedHandle diskAdaptor(new MultiDiskAdaptor()); - diskAdaptor->setFileEntries(std::deque >(&fs[0], &fs[arrayLength(fs)])); + diskAdaptor->setFileEntries + (std::deque >(&fs[0], &fs[arrayLength(fs)])); diskAdaptor->setPieceLength(1024); diskAdaptor->setStoreDir(storeDir); + diskAdaptor->setTopDir(topDir); diskAdaptor->openFile(); SharedHandle itr - (dynamic_pointer_cast(diskAdaptor->fileAllocationIterator())); + (dynamic_pointer_cast + (diskAdaptor->fileAllocationIterator())); DiskWriterEntries entries = itr->getDiskWriterEntries(); - std::sort(entries.begin(), entries.end()); + CPPUNIT_ASSERT_EQUAL((size_t)11, entries.size()); - CPPUNIT_ASSERT_EQUAL((size_t)8, entries.size()); - - CPPUNIT_ASSERT_EQUAL(storeDir+std::string("/file1"), entries[0]->getFilePath(storeDir)); - CPPUNIT_ASSERT_EQUAL(storeDir+std::string("/file2"), entries[1]->getFilePath(storeDir)); - CPPUNIT_ASSERT_EQUAL(storeDir+std::string("/file3"), entries[2]->getFilePath(storeDir)); - CPPUNIT_ASSERT_EQUAL(storeDir+std::string("/file6"), entries[3]->getFilePath(storeDir)); - CPPUNIT_ASSERT_EQUAL(storeDir+std::string("/file7"), entries[4]->getFilePath(storeDir)); - CPPUNIT_ASSERT_EQUAL(storeDir+std::string("/file8"), entries[5]->getFilePath(storeDir)); - CPPUNIT_ASSERT_EQUAL(storeDir+std::string("/file9"), entries[6]->getFilePath(storeDir)); - CPPUNIT_ASSERT_EQUAL(storeDir+std::string("/fileB"), entries[7]->getFilePath(storeDir)); + // file1 + CPPUNIT_ASSERT_EQUAL(prefix+std::string("/file1"), + entries[0]->getFilePath(prefix)); + CPPUNIT_ASSERT(entries[0]->needsFileAllocation()); + CPPUNIT_ASSERT(!entries[0]->getDiskWriter().isNull()); + // file2 + CPPUNIT_ASSERT_EQUAL(prefix+std::string("/file2"), + entries[1]->getFilePath(prefix)); + CPPUNIT_ASSERT(entries[1]->needsFileAllocation()); + CPPUNIT_ASSERT(!entries[1]->getDiskWriter().isNull()); + // file3 + CPPUNIT_ASSERT_EQUAL(prefix+std::string("/file3"), + entries[2]->getFilePath(prefix)); + CPPUNIT_ASSERT(entries[2]->needsFileAllocation()); + CPPUNIT_ASSERT(!entries[2]->getDiskWriter().isNull()); + // file4, diskWriter is not null, because file exists. + CPPUNIT_ASSERT_EQUAL(prefix+std::string("/file4"), + entries[3]->getFilePath(prefix)); + CPPUNIT_ASSERT(!entries[3]->needsFileAllocation()); + CPPUNIT_ASSERT(!entries[3]->getDiskWriter().isNull()); + // file5 + CPPUNIT_ASSERT_EQUAL(prefix+std::string("/file5"), + entries[4]->getFilePath(prefix)); + CPPUNIT_ASSERT(!entries[4]->needsFileAllocation()); + CPPUNIT_ASSERT(entries[4]->getDiskWriter().isNull()); + // file6 + CPPUNIT_ASSERT_EQUAL(prefix+std::string("/file6"), + entries[5]->getFilePath(prefix)); + CPPUNIT_ASSERT(entries[5]->needsFileAllocation()); + CPPUNIT_ASSERT(!entries[5]->getDiskWriter().isNull()); + // file7 + CPPUNIT_ASSERT_EQUAL(prefix+std::string("/file7"), + entries[6]->getFilePath(prefix)); + CPPUNIT_ASSERT(entries[6]->needsFileAllocation()); + CPPUNIT_ASSERT(!entries[6]->getDiskWriter().isNull()); + // file8 + CPPUNIT_ASSERT_EQUAL(prefix+std::string("/file8"), + entries[7]->getFilePath(prefix)); + CPPUNIT_ASSERT(entries[7]->needsFileAllocation()); + CPPUNIT_ASSERT(!entries[7]->getDiskWriter().isNull()); + // file9 + CPPUNIT_ASSERT_EQUAL(prefix+std::string("/file9"), + entries[8]->getFilePath(prefix)); + CPPUNIT_ASSERT(entries[8]->needsFileAllocation()); + CPPUNIT_ASSERT(!entries[8]->getDiskWriter().isNull()); + // fileA + CPPUNIT_ASSERT_EQUAL(prefix+std::string("/fileA"), + entries[9]->getFilePath(prefix)); + CPPUNIT_ASSERT(!entries[9]->needsFileAllocation()); + CPPUNIT_ASSERT(entries[9]->getDiskWriter().isNull()); + // fileB + CPPUNIT_ASSERT_EQUAL(prefix+std::string("/fileB"), + entries[10]->getFilePath(prefix)); + CPPUNIT_ASSERT(entries[10]->needsFileAllocation()); + CPPUNIT_ASSERT(!entries[10]->getDiskWriter().isNull()); } void MultiFileAllocationIteratorTest::testAllocate() { std::string dir = "/tmp"; std::string topDir = "aria2_MultiFileAllocationIteratorTest_testAllocate"; + std::string prefix = dir+"/"+topDir; std::string fname1 = "file1"; std::string fname2 = "file2"; std::string fname3 = "file3"; @@ -98,6 +156,7 @@ void MultiFileAllocationIteratorTest::testAllocate() SharedHandle diskAdaptor(new MultiDiskAdaptor()); diskAdaptor->setStoreDir(dir); diskAdaptor->setTopDir(topDir); + diskAdaptor->setPieceLength(1); int64_t offset = 0; SharedHandle fileEntry1(new FileEntry(fname1, @@ -139,20 +198,29 @@ void MultiFileAllocationIteratorTest::testAllocate() fs.push_back(fileEntry6); diskAdaptor->setFileEntries(fs); + + File(prefix+"/"+fname1).remove(); + File(prefix+"/"+fname2).remove(); + File(prefix+"/"+fname3).remove(); + File(prefix+"/"+fname4).remove(); + File(prefix+"/"+fname5).remove(); + File(prefix+"/"+fname6).remove(); + // we have to open file first. diskAdaptor->initAndOpenFile(); SharedHandle itr - (dynamic_pointer_cast(diskAdaptor->fileAllocationIterator())); + (dynamic_pointer_cast + (diskAdaptor->fileAllocationIterator())); while(!itr->finished()) { itr->allocateChunk(); } - CPPUNIT_ASSERT_EQUAL((uint64_t)length1, File(dir+"/"+topDir+"/"+fname1).size()); - CPPUNIT_ASSERT_EQUAL((uint64_t)length2, File(dir+"/"+topDir+"/"+fname2).size()); - CPPUNIT_ASSERT_EQUAL((uint64_t)length3, File(dir+"/"+topDir+"/"+fname3).size()); - CPPUNIT_ASSERT(!File(dir+"/"+topDir+"/"+fname4).isFile()); + CPPUNIT_ASSERT_EQUAL((uint64_t)length1, File(prefix+"/"+fname1).size()); + CPPUNIT_ASSERT_EQUAL((uint64_t)length2, File(prefix+"/"+fname2).size()); + CPPUNIT_ASSERT_EQUAL((uint64_t)length3, File(prefix+"/"+fname3).size()); + CPPUNIT_ASSERT(!File(prefix+"/"+fname4).isFile()); - CPPUNIT_ASSERT_EQUAL((uint64_t)length5, File(dir+"/"+topDir+"/"+fname5).size()); - CPPUNIT_ASSERT(!File(dir+"/"+topDir+"/"+fname6).isFile()); + CPPUNIT_ASSERT_EQUAL((uint64_t)length5, File(prefix+"/"+fname5).size()); + CPPUNIT_ASSERT(!File(prefix+"/"+fname6).isFile()); } catch(Exception& e) { CPPUNIT_FAIL(e.stackTrace());