Merge pull request #889 from openzim/test_reader

Add some unittest on get_buffer

src/buffer_reader.cpp

@@ -22,6 +22,7 @@
 #include "buffer_reader.h"
 #include "buffer.h"
 
+#include <stdexcept>
 #include <cstring>
 
 namespace zim {
@@ -49,18 +50,12 @@ offset_t BufferReader::offset() const
 }
 
 
-void BufferReader::read(char* dest, offset_t offset, zsize_t size) const {
- ASSERT(offset.v, <=, source.size().v);
- ASSERT(offset+offset_t(size.v), <=, offset_t(source.size().v));
- if (! size ) {
- return;
- }
+void BufferReader::readImpl(char* dest, offset_t offset, zsize_t size) const {
  memcpy(dest, source.data(offset), size.v);
 }
 
 
-char BufferReader::read(offset_t offset) const {
- ASSERT(offset.v, <, source.size().v);
+char BufferReader::readImpl(offset_t offset) const {
  char dest;
  dest = *source.data(offset);
  return dest;

src/buffer_reader.h

@@ -30,15 +30,17 @@ class LIBZIM_PRIVATE_API BufferReader : public Reader {
  : source(source) {}
  virtual ~BufferReader() {};
 
- zsize_t size() const;
- offset_t offset() const;
+ zsize_t size() const override;
+ offset_t offset() const override;
 
- void read(char* dest, offset_t offset, zsize_t size) const;
- char read(offset_t offset) const;
- const Buffer get_buffer(offset_t offset, zsize_t size) const;
- std::unique_ptr<const Reader> sub_reader(offset_t offset, zsize_t size) const;
+ const Buffer get_buffer(offset_t offset, zsize_t size) const override;
+ std::unique_ptr<const Reader> sub_reader(offset_t offset, zsize_t size) const override;
 
- private:
+ private: // functions
+ void readImpl(char* dest, offset_t offset, zsize_t size) const override;
+ char readImpl(offset_t offset) const override;
+
+ private: // data
  const Buffer source;
 };
 

src/file_reader.cpp

@@ -30,6 +30,7 @@
 #include <fcntl.h>
 #include <system_error>
 #include <algorithm>
+#include <stdexcept>
 
 
 #ifndef _WIN32
@@ -43,6 +44,19 @@
  typedef SSIZE_T ssize_t;
 #endif
 
+namespace {
+ [[noreturn]] void throwSystemError(const std::string& errorText)
+ {
+#ifdef _WIN32
+ // Windows doesn't use errno.
+ throw std::system_error(std::error_code(), errorText);
+#else
+ std::error_code ec(errno, std::generic_category());
+ throw std::system_error(ec, errorText);
+#endif
+ }
+}
+
 namespace zim {
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -60,8 +74,7 @@ MultiPartFileReader::MultiPartFileReader(std::shared_ptr<const FileCompound> sou
  ASSERT(offset.v+size.v, <=, source->fsize().v);
 }
 
-char MultiPartFileReader::read(offset_t offset) const {
- ASSERT(offset.v, <, _size.v);
+char MultiPartFileReader::readImpl(offset_t offset) const {
  offset += _offset;
  auto part_pair = source->locate(offset);
  auto& fhandle = part_pair->second->fhandle();
@@ -81,19 +94,13 @@ char MultiPartFileReader::read(offset_t offset) const {
  fmt << " - Reading offset at " << offset.v << "\n";
  fmt << " - logical local offset is " << logical_local_offset.v << "\n";
  fmt << " - physical local offset is " << physical_local_offset.v << "\n";
- fmt << " - error is " << strerror(errno) << "\n";
- std::error_code ec(errno, std::generic_category());
- throw std::system_error(ec, fmt);
+ fmt << " - error is " << e.what() << "\n";
+ throwSystemError(fmt);
  };
  return ret;
 }
 
-void MultiPartFileReader::read(char* dest, offset_t offset, zsize_t size) const {
- ASSERT(offset.v, <=, _size.v);
- ASSERT(offset.v+size.v, <=, _size.v);
- if (! size ) {
- return;
- }
+void MultiPartFileReader::readImpl(char* dest, offset_t offset, zsize_t size) const {
  offset += _offset;
  auto found_range = source->locate(offset, size);
  for(auto current = found_range.first; current!=found_range.second; current++){
@@ -116,9 +123,8 @@ void MultiPartFileReader::read(char* dest, offset_t offset, zsize_t size) const
  fmt << " - Reading offset at " << offset.v << "\n";
  fmt << " - logical local offset is " << logical_local_offset.v << "\n";
  fmt << " - physical local offset is " << physical_local_offset.v << "\n";
- fmt << " - error is " << strerror(errno) << "\n";
- std::error_code ec(errno, std::generic_category());
- throw std::system_error(ec, fmt);
+ fmt << " - error is " << e.what() << "\n";
+ throwSystemError(fmt);
  };
  ASSERT(size_to_get, <=, size);
  dest += size_to_get.v;
@@ -244,9 +250,8 @@ FileReader::FileReader(FileHandle fh, offset_t offset, zsize_t size)
 {
 }
 
-char FileReader::read(offset_t offset) const
+char FileReader::readImpl(offset_t offset) const
 {
- ASSERT(offset.v, <, _size.v);
  offset += _offset;
  char ret;
  try {
@@ -256,20 +261,14 @@ char FileReader::read(offset_t offset) const
  Formatter fmt;
  fmt << "Cannot read a char.\n";
  fmt << " - Reading offset at " << offset.v << "\n";
- fmt << " - error is " << strerror(errno) << "\n";
- std::error_code ec(errno, std::generic_category());
- throw std::system_error(ec, fmt);
+ fmt << " - error is " << e.what() << "\n";
+ throwSystemError(fmt);
  };
  return ret;
 }
 
-void FileReader::read(char* dest, offset_t offset, zsize_t size) const
+void FileReader::readImpl(char* dest, offset_t offset, zsize_t size) const
 {
- ASSERT(offset.v, <=, _size.v);
- ASSERT(offset.v+size.v, <=, _size.v);
- if (! size ) {
- return;
- }
  offset += _offset;
  try {
  _fhandle->readAt(dest, size, offset);
@@ -278,9 +277,8 @@ void FileReader::read(char* dest, offset_t offset, zsize_t size) const
  fmt << "Cannot read chars.\n";
  fmt << " - Reading offset at " << offset.v << "\n";
  fmt << " - size is " << size.v << "\n";
- fmt << " - error is " << strerror(errno) << "\n";
- std::error_code ec(errno, std::generic_category());
- throw std::system_error(ec, fmt);
+ fmt << " - error is " << e.what() << "\n";
+ throwSystemError(fmt);
  };
 }
 

src/file_reader.h

@@ -33,12 +33,12 @@ class LIBZIM_PRIVATE_API BaseFileReader : public Reader {
  BaseFileReader(offset_t offset, zsize_t size)
  : _offset(offset), _size(size) {}
  ~BaseFileReader() = default;
- zsize_t size() const { return _size; };
- offset_t offset() const { return _offset; };
+ zsize_t size() const override { return _size; };
+ offset_t offset() const override { return _offset; };
 
  virtual const Buffer get_mmap_buffer(offset_t offset,
  zsize_t size) const = 0;
- const Buffer get_buffer(offset_t offset, zsize_t size) const;
+ const Buffer get_buffer(offset_t offset, zsize_t size) const override;
 
  protected: // data
  offset_t _offset;
@@ -53,11 +53,12 @@ class LIBZIM_PRIVATE_API FileReader : public BaseFileReader {
  FileReader(FileHandle fh, offset_t offset, zsize_t size);
  ~FileReader() = default;
 
- char read(offset_t offset) const;
- void read(char *dest, offset_t offset, zsize_t size) const;
+ const Buffer get_mmap_buffer(offset_t offset, zsize_t size) const override;
+ std::unique_ptr<const Reader> sub_reader(offset_t offset, zsize_t size) const override;
 
- const Buffer get_mmap_buffer(offset_t offset, zsize_t size) const;
- std::unique_ptr<const Reader> sub_reader(offset_t offset, zsize_t size) const;
+ private: // functions
+ char readImpl(offset_t offset) const override;
+ void readImpl(char *dest, offset_t offset, zsize_t size) const override;
 
  private: // data
  // The file handle is stored via a shared pointer so that it can be shared
@@ -71,13 +72,14 @@ class LIBZIM_PRIVATE_API MultiPartFileReader : public BaseFileReader {
  explicit MultiPartFileReader(std::shared_ptr<const FileCompound> source);
  ~MultiPartFileReader() {};
 
- char read(offset_t offset) const;
- void read(char *dest, offset_t offset, zsize_t size) const;
+ const Buffer get_mmap_buffer(offset_t offset, zsize_t size) const override;
+ std::unique_ptr<const Reader> sub_reader(offset_t offset, zsize_t size) const override;
 
- const Buffer get_mmap_buffer(offset_t offset, zsize_t size) const;
- std::unique_ptr<const Reader> sub_reader(offset_t offset, zsize_t size) const;
+ private: // functions
+ char readImpl(offset_t offset) const override;
+ void readImpl(char *dest, offset_t offset, zsize_t size) const override;
 
- private:
+ private: // data
  MultiPartFileReader(std::shared_ptr<const FileCompound> source, offset_t offset, zsize_t size);
 
  std::shared_ptr<const FileCompound> source;

src/fs_unix.cpp

@@ -47,8 +47,11 @@ zsize_t FD::readAt(char* dest, zsize_t size, offset_t offset) const
  errno = 0;
  while (size_to_read > 0) {
  auto size_read = PREAD(m_fd, dest, size_to_read, current_offset);
+ if (size_read == 0) {
+ throw std::runtime_error("Cannot read past the end of the file");
+ }
  if (size_read == -1) {
- return zsize_t(-1);
+ throw std::runtime_error("Cannot read file");
  }
  size_to_read -= size_read;
  current_offset += size_read;

src/fs_windows.cpp

@@ -69,26 +69,41 @@ FD::~FD()
 zsize_t FD::readAt(char* dest, zsize_t size, offset_t offset) const
 {
  if (!mp_impl)
- return zsize_t(-1);
+ throw std::runtime_error("FD is not open");
  EnterCriticalSection(&mp_impl->m_criticalSection);
  LARGE_INTEGER off;
  off.QuadPart = offset.v;
+ std::string errorMsg;
+ auto size_to_read = size.v;
+
  if (!SetFilePointerEx(mp_impl->m_handle, off, NULL, FILE_BEGIN)) {
+ errorMsg = "Seek fail";
  goto err;
  }
 
  DWORD size_read;
- if (!ReadFile(mp_impl->m_handle, dest, size.v, &size_read, NULL)) {
- goto err;
- }
- if (size_read != size.v) {
- goto err;
+ while (size_to_read > 0) {
+ // Read by batch < 4GiB
+ // Lets use a batch of 1GiB
+ auto batch_to_read = std::min(size_to_read, (size_type)1024*1024*1024);
+ if (!ReadFile(mp_impl->m_handle, dest, batch_to_read, &size_read, NULL)) {
+ errorMsg = "Read fail";
+ goto err;
+ }
+
+ if (size_read == 0) {
+ errorMsg = "Cannot read past the end of the file";
+ goto err;
+ }
+
+ size_to_read -= size_read;
+ dest += size_read;
  }
  LeaveCriticalSection(&mp_impl->m_criticalSection);
  return size;
 err:
  LeaveCriticalSection(&mp_impl->m_criticalSection);
- return zsize_t(-1);
+ throw std::runtime_error(errorMsg);
 }
 
 bool FD::seek(offset_t offset)

src/reader.h

@@ -22,6 +22,7 @@
 #define ZIM_READER_H_
 
 #include <memory>
+#include <stdexcept>
 
 #include "zim_types.h"
 #include "endian_tools.h"
@@ -31,13 +32,23 @@
 
 namespace zim {
 
-class Reader {
+class LIBZIM_PRIVATE_API Reader {
  public:
  Reader() {};
  virtual zsize_t size() const = 0;
  virtual ~Reader() {};
 
- virtual void read(char* dest, offset_t offset, zsize_t size) const = 0;
+ void read(char* dest, offset_t offset, zsize_t size) const {
+ if (can_read(offset, size)) {
+ if (size) {
+ // Do the actuall read only if we have a size to read
+ readImpl(dest, offset, size);
+ }
+ return;
+ }
+ throw std::runtime_error("Cannot read after the end of the reader");
+ }
+
  template<typename T>
  T read_uint(offset_t offset) const {
  ASSERT(offset.v, <, size().v);
@@ -46,7 +57,13 @@ class Reader {
  read(tmp_buf, offset, zsize_t(sizeof(T)));
  return fromLittleEndian<T>(tmp_buf);
  }
- virtual char read(offset_t offset) const = 0;
+
+ char read(offset_t offset) const {
+ if (can_read(offset, zsize_t(1))) {
+ return readImpl(offset);
+ }
+ throw std::runtime_error("Cannot read after the end of the reader");
+ }
 
  virtual const Buffer get_buffer(offset_t offset, zsize_t size) const = 0;
  const Buffer get_buffer(offset_t offset) const {
@@ -59,6 +76,15 @@ class Reader {
  virtual offset_t offset() const = 0;
 
  bool can_read(offset_t offset, zsize_t size) const;
+
+ private:
+ // Implementation of the read method.
+ // Check of the validity of the offset/size has already been done.
+ virtual void readImpl(char* dest, offset_t offset, zsize_t size) const = 0;
+
+ // Implementation of the read method.
+ // Check of the validity of the offset has already been done.
+ virtual char readImpl(offset_t offset) const = 0;
 };
 
 };