Merge pull request #1178 from Exiv2/0.27_fileio_performance

Reduce system calls in FileIo::eof() (master -> 0.27)

samples/largeiptc-test.cpp

@@ -3,11 +3,12 @@
 
 #include <exiv2/exiv2.hpp>
 
-#include <iostream>
 #include <cassert>
+#include <iostream>
 
 int main(int argc, char* const argv[])
-try {
+{
+    try {
         Exiv2::XmpParser::initialize();
         ::atexit(Exiv2::XmpParser::terminate);
 
@@ -23,10 +24,12 @@ try {
         if (io.open() != 0) {
             throw Exiv2::Error(Exiv2::kerDataSourceOpenFailed, io.path(), Exiv2::strError());
         }
-    Exiv2::DataBuf buf((long)io.size());
+        Exiv2::DataBuf buf(static_cast<long>(io.size()));
         std::cout << "Reading " << buf.size_ << " bytes from " << data << "\n";
-    io.read(buf.pData_, buf.size_);
+        long readBytes = io.read(buf.pData_, buf.size_);
-    if (io.error() || !io.eof()) throw Exiv2::Error(Exiv2::kerFailedToReadImageData);
+        if (readBytes != buf.size_ || io.error() || io.eof()) {
+            throw Exiv2::Error(Exiv2::kerFailedToReadImageData);
+        }
 
         // Read metadata from file
         Exiv2::Image::AutoPtr image = Exiv2::ImageFactory::open(file);
@@ -65,8 +68,8 @@ try {
         image->writeMetadata();
 
         return 0;
-}
+    } catch (Exiv2::AnyError& e) {
-catch (Exiv2::AnyError& e) {
         std::cout << "Caught Exiv2 exception '" << e << "'\n";
         return -1;
+    }
 }

src/basicio.cpp

@@ -1022,8 +1022,7 @@ namespace Exiv2 {
 
     bool FileIo::eof() const
     {
-        assert(p_->fp_ != 0);
+        return std::feof(p_->fp_) != 0;
-        return feof(p_->fp_) != 0 || tell() >= (long) size() ;
     }
 
     std::string FileIo::path() const

tests/bugfixes/github/test_issue_428.py

@@ -13,19 +13,22 @@ class PngReadRawProfile(metaclass=system_tests.CaseMeta):
 
     filenames = [
         system_tests.path("$data_path/issue_428_poc1.png"),
-        system_tests.path("$data_path/issue_428_poc2.png"),
         system_tests.path("$data_path/issue_428_poc3.png"),
         system_tests.path("$data_path/issue_428_poc4.png"),
         system_tests.path("$data_path/issue_428_poc5.png"),
         system_tests.path("$data_path/issue_428_poc8.png"),
 
+        system_tests.path("$data_path/issue_428_poc2.png"),
         system_tests.path("$data_path/issue_428_poc6.png"),
         system_tests.path("$data_path/issue_428_poc7.png"),
     ]
 
     commands = ["$exiv2 " + fname for fname in filenames]
     stdout = [""] * len(filenames)
-    stderr = [ stderr_exception(fname) for fname in filenames[0:6] ]
+    stderr = [ stderr_exception(fname) for fname in filenames[0:5] ]
+    stderr.append("""$exiv2_exception_message """ + filenames[5] + """:
+$kerInputDataReadFailed
+""")
     stderr.append("""Error: XMP Toolkit error 201: XML parsing failure
 Warning: Failed to decode XMP metadata.
 """ + stderr_exception(filenames[6]))

tests/suite.conf

@@ -22,6 +22,7 @@ taglist: ${ENV:exiv2_path}/taglist${ENV:binary_extension}
 [variables]
 kerOffsetOutOfRange: Offset out of range
 kerFailedToReadImageData: Failed to read image data
+kerInputDataReadFailed: Failed to read input data
 kerCorruptedMetadata: corrupted image metadata
 kerInvalidMalloc: invalid memory allocation request
 kerInvalidTypeValue: invalid type in tiff structure

unitTests/CMakeLists.txt

@@ -19,23 +19,28 @@ endforeach()
 
 add_executable(unit_tests mainTestRunner.cpp
     gtestwrapper.h
-    test_basicio.cpp
-    test_types.cpp
-    test_tiffheader.cpp
-    test_futils.cpp
-    test_enforce.cpp
-    test_safe_op.cpp
-    test_XmpKey.cpp
     test_DateValue.cpp
     test_TimeValue.cpp
+    test_XmpKey.cpp
+    test_basicio.cpp
     test_cr2header_int.cpp
+    test_enforce.cpp
+    test_FileIo.cpp
+    test_futils.cpp
     test_helper_functions.cpp
-    test_slice.cpp
     test_image_int.cpp
+    test_safe_op.cpp
+    test_slice.cpp
+    test_tiffheader.cpp
+    test_types.cpp
     ${unit_tests_exiv2lib_SOURCES}
 )
 
-target_compile_definitions(unit_tests PRIVATE exiv2lib_STATIC)
+target_compile_definitions(unit_tests
+  PRIVATE
+    exiv2lib_STATIC
+    TESTDATA_PATH="${PROJECT_SOURCE_DIR}/test/data"
+)
 
 if (exiv2lib_COMPILE_DEFINITIONS)
     target_compile_definitions(unit_tests PRIVATE ${exiv2lib_COMPILE_DEFINITIONS})

unitTests/test_FileIo.cpp

@@ -0,0 +1,74 @@
+#include "basicio.hpp"
+
+#include <gtest/gtest.h>
+
+using namespace Exiv2;
+
+namespace
+{
+    const std::string testData(TESTDATA_PATH);
+    const std::string imagePath(testData + "/DSC_3079.jpg");
+}  // namespace
+
+TEST(AFileIO, canBeInstantiatedWithFilePath)
+{
+    ASSERT_NO_THROW(FileIo file(imagePath));
+}
+
+TEST(AFileIO, canBeOpenInReadBinaryMode)
+{
+    FileIo file(imagePath);
+    ASSERT_EQ(0, file.open());
+}
+
+TEST(AFileIO, isOpenDoItsJob)
+{
+    FileIo file(imagePath);
+    ASSERT_FALSE(file.isopen());
+    file.open();
+    ASSERT_TRUE(file.isopen());
+}
+
+TEST(AFileIO, returnsFileSizeIfItsOpened)
+{
+    FileIo file(imagePath);
+    file.open();
+    ASSERT_EQ(118685ul, file.size());
+}
+
+TEST(AFileIO, returnsFileSizeEvenWhenFileItIsNotOpened)
+{
+    FileIo file(imagePath);
+    ASSERT_EQ(118685ul, file.size());
+}
+
+TEST(AFileIO, isOpenedAtPosition0)
+{
+    FileIo file(imagePath);
+    file.open();
+    ASSERT_EQ(0, file.tell());
+}
+
+TEST(AFileIO, canSeekToExistingPositions)
+{
+    FileIo file(imagePath);
+    file.open();
+
+    ASSERT_EQ(0, file.seek(100, BasicIo::beg));
+    ASSERT_EQ(0, file.seek(-50, BasicIo::cur));
+    ASSERT_EQ(0, file.seek(-50, BasicIo::end));
+
+    ASSERT_FALSE(file.error());
+    ASSERT_FALSE(file.eof());
+}
+
+TEST(AFileIO, canSeekBeyondEOF)
+{
+    FileIo file(imagePath);
+    file.open();
+
+    // POSIX allows seeking beyond the existing end of file.
+    ASSERT_EQ(0, file.seek(200000, BasicIo::beg));
+    ASSERT_FALSE(file.error());
+    ASSERT_FALSE(file.eof());
+}