2008-09-07 Tatsuhiro Tsujikawa <tujikawa at rednoah dot com>

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.
2008-09-07 11:37:15 +00:00 · 2008-09-07 11:37:15 +00:00 · 4e28efd925
parent 390538d0b5
commit 4e28efd925
6 changed files with 178 additions and 72 deletions
--- a/12
+++ b/12
@ -1,3 +1,15 @@
+2008-09-07  Tatsuhiro Tsujikawa  <tujikawa at rednoah dot com>
+
+	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  <tujikawa at rednoah dot com>

 	Fixed the bug that exception is thrown when MultiDiskAdaptor::size() is
--- a/src/MultiDiskAdaptor.cc
+++ b/src/MultiDiskAdaptor.cc
@ -45,11 +45,13 @@
 #include "Logger.h"
 #include "SimpleRandomizer.h"
 #include <algorithm>
+#include <cassert>

 namespace aria2 {

 DiskWriterEntry::DiskWriterEntry(const SharedHandle<FileEntry>& 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<DiskWriterEntry> createDiskWriterEntry
 (const SharedHandle<FileEntry>& fileEntry,
- DiskWriterFactory& dwFactory,
- bool directIOAllowed)
+ bool needsFileAllocation)
 {
  SharedHandle<DiskWriterEntry> 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<SharedHandle<FileEntry> >::const_iterator itr =
-	  fileEntries.begin(); itr != fileEntries.end(); ++itr) {
-      if((*itr)->isRequested()) {
-	diskWriterEntries.push_back
-	  (createDiskWriterEntry(*itr, dwFactory, _directIOAllowed));
-      }
-    }
-  } else {
-    std::deque<SharedHandle<FileEntry> >::const_iterator done = fileEntries.begin();
-    for(std::deque<SharedHandle<FileEntry> >::const_iterator itr =
-	  fileEntries.begin(); itr != fileEntries.end();) {
-      if(!(*itr)->isRequested()) {
+  for(std::deque<SharedHandle<FileEntry> >::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<SharedHandle<DiskWriterEntry> >::iterator done =
+      diskWriterEntries.begin();
+    for(std::deque<SharedHandle<DiskWriterEntry> >::iterator itr =
+	  diskWriterEntries.begin(); itr != diskWriterEntries.end();) {
+      if(!(*itr)->getFileEntry()->isRequested()) {
 	++itr;
 	continue;
      }
-      off_t pieceStartOffset = ((*itr)->getOffset()/pieceLength)*pieceLength;
-      std::deque<SharedHandle<DiskWriterEntry> >::iterator insertionPoint =
-	diskWriterEntries.end();
-
-      if(itr != fileEntries.begin()) {
-	for(std::deque<SharedHandle<FileEntry> >::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<SharedHandle<DiskWriterEntry> >::iterator i =
+	      itr-1; i != done; --i) {
+	  const SharedHandle<FileEntry>& 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<SharedHandle<DiskWriterEntry> >::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
--- a/src/MultiDiskAdaptor.h
+++ b/src/MultiDiskAdaptor.h
@ -49,6 +49,7 @@ private:
  SharedHandle<DiskWriter> diskWriter;
  bool _open;
  bool _directIO;
+  bool _needsFileAllocation;
 public:
  DiskWriterEntry(const SharedHandle<FileEntry>& 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<DiskWriterEntry> DiskWriterEntryHandle;
--- 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(),
--- 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();

--- 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 <algorithm>
 #include <iostream>
 #include <cppunit/extensions/HelperMacros.h>
@ -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<MultiDiskAdaptor> diskAdaptor(new MultiDiskAdaptor());
-  diskAdaptor->setFileEntries(std::deque<SharedHandle<FileEntry> >(&fs[0], &fs[arrayLength(fs)]));
+  diskAdaptor->setFileEntries
+    (std::deque<SharedHandle<FileEntry> >(&fs[0], &fs[arrayLength(fs)]));
  diskAdaptor->setPieceLength(1024);
  diskAdaptor->setStoreDir(storeDir);
+  diskAdaptor->setTopDir(topDir);
  diskAdaptor->openFile();

  SharedHandle<MultiFileAllocationIterator> itr
-    (dynamic_pointer_cast<MultiFileAllocationIterator>(diskAdaptor->fileAllocationIterator()));
+    (dynamic_pointer_cast<MultiFileAllocationIterator>
+     (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<MultiDiskAdaptor> diskAdaptor(new MultiDiskAdaptor());
    diskAdaptor->setStoreDir(dir);
    diskAdaptor->setTopDir(topDir);
+    diskAdaptor->setPieceLength(1);

    int64_t offset = 0;
    SharedHandle<FileEntry> 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<MultiFileAllocationIterator> itr
-      (dynamic_pointer_cast<MultiFileAllocationIterator>(diskAdaptor->fileAllocationIterator()));
+      (dynamic_pointer_cast<MultiFileAllocationIterator>
+       (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());