From f0a016dcb150e1f96b39f6d6a82a5f50c6e6d921 Mon Sep 17 00:00:00 2001 From: Augustin Cavalier Date: Thu, 21 Sep 2023 22:59:29 -0400 Subject: [PATCH] BBufferedDataIO: Fix Write(). I looked at this code as a reference when writing similar code in another project, and realized then that this was broken to the point where I don't think anyone could have actually used it: 1. Writes larger than the buffer should only return the amount written by this Write() call, not the amount flushed beforehand. 2. fDirty was only set if there were bytes remaining after the first write to the buffer. So, if you wrote small amounts of data, they would be silently discarded most of the time! 3. When filling the buffer, we might as well flush simultaneously. This allows the logic to be consolidated into a loop and remove the duplicate memcpy. In case of failure, just return the bytes-written as the write could be retried later. 4. Flush() should always return an error or 0, not bytes written. While at it, add some basic tests for this class. Change-Id: I2de01d0b31e3fe22863cef21dd7b0b62ed47121b --- src/kits/support/BufferedDataIO.cpp | 59 ++++++++----------- src/tests/kits/storage/DataIOTest.cpp | 46 +++++++++++++++ src/tests/kits/storage/DataIOTest.h | 23 ++++++++ src/tests/kits/storage/Jamfile | 1 + .../kits/storage/StorageKitTestAddon.cpp | 2 + 5 files changed, 97 insertions(+), 34 deletions(-) create mode 100644 src/tests/kits/storage/DataIOTest.cpp create mode 100644 src/tests/kits/storage/DataIOTest.h diff --git a/src/kits/support/BufferedDataIO.cpp b/src/kits/support/BufferedDataIO.cpp index 7fb3c8f017..1978004d90 100644 --- a/src/kits/support/BufferedDataIO.cpp +++ b/src/kits/support/BufferedDataIO.cpp @@ -95,10 +95,10 @@ BBufferedDataIO::Flush() } else if (bytesWritten >= 0) { fSize -= bytesWritten; fPosition += bytesWritten; - return B_ERROR; + return B_PARTIAL_WRITE; } - return bytesWritten; + return B_OK; } @@ -172,6 +172,15 @@ BBufferedDataIO::Write(const void* buffer, size_t size) TRACE("%p::Write(size %lu)\n", this, size); + if (size > fBufferSize || fBuffer == NULL) { + // request is larger than our buffer, just fill it directly + status_t status = Flush(); + if (status != B_OK) + return status; + + return fStream.Write(buffer, size); + } + if (!fDirty) { // Throw away a read-only buffer if necessary TRACE("%p: throw away previous buffer.\n", this); @@ -180,39 +189,21 @@ BBufferedDataIO::Write(const void* buffer, size_t size) } size_t bytesWritten = 0; - - if (size > fBufferSize || fBuffer == NULL) { - // request is larger than our buffer, just fill it directly - bytesWritten = fSize; - - status_t status = Flush(); - if (status != B_OK) - return status; - - ssize_t streamWritten = fStream.Write(buffer, size); - if (streamWritten >= 0) - return bytesWritten + streamWritten; - - return streamWritten; - } - - bytesWritten = min_c(size, fBufferSize - fSize - fPosition); - TRACE("%p: write %" B_PRIuSIZE " bytes to the buffer.\n", this, - bytesWritten); - memcpy(fBuffer + fPosition + fSize, buffer, bytesWritten); - fSize += bytesWritten; - size -= bytesWritten; - - if (size > 0) { - status_t status = Flush(); - if (status != B_OK) - return status; - - memcpy(fBuffer, (uint8*)buffer + bytesWritten, size); - fPosition = 0; - fSize = size; + while (size > 0) { + size_t toCopy = min_c(size, fBufferSize - (fPosition + fSize)); + TRACE("%p: write %" B_PRIuSIZE " bytes to the buffer.\n", this, + toCopy); + memcpy(fBuffer + (fPosition + fSize), buffer, toCopy); + fSize += toCopy; + bytesWritten += toCopy; + size -= toCopy; fDirty = true; - bytesWritten += size; + + if ((fPosition + fSize) == fBufferSize) { + status_t status = Flush(); + if (status != B_OK) + return bytesWritten; + } } return bytesWritten; diff --git a/src/tests/kits/storage/DataIOTest.cpp b/src/tests/kits/storage/DataIOTest.cpp new file mode 100644 index 0000000000..ae8a63ecb3 --- /dev/null +++ b/src/tests/kits/storage/DataIOTest.cpp @@ -0,0 +1,46 @@ +// DataIOTest.cpp + +#include +#include + +#include + +#include "DataIOTest.h" + + +CppUnit::Test* +DataIOTest::Suite() { + CppUnit::TestSuite *suite = new CppUnit::TestSuite(); + typedef CppUnit::TestCaller TC; + + suite->addTest(new TC("BResourceString::BufferedDataIO Test", + &DataIOTest::BufferedDataIOTest)); + + return suite; +} + + +void +DataIOTest::BufferedDataIOTest() +{ + // very basic test + NextSubTest(); + { + BMallocIO mallocIO; + CPPUNIT_ASSERT(mallocIO.SetSize(1024) == B_OK); + + BBufferedDataIO bufferedDataIO(mallocIO, 8, false); + CPPUNIT_ASSERT(bufferedDataIO.InitCheck() == B_OK); + + CPPUNIT_ASSERT(bufferedDataIO.Write("test ", 5) == 5); + CPPUNIT_ASSERT(bufferedDataIO.Write("test ", 5) == 5); + CPPUNIT_ASSERT(bufferedDataIO.Write("test ", 5) == 5); + + CPPUNIT_ASSERT(bufferedDataIO.Flush() == B_OK); + + CPPUNIT_ASSERT(bufferedDataIO.Write("longer-test", 12) == 12); + + CPPUNIT_ASSERT(mallocIO.Position() == 27); + CPPUNIT_ASSERT(memcmp(mallocIO.Buffer(), "test test test longer-test", 27) == 0); + } +} diff --git a/src/tests/kits/storage/DataIOTest.h b/src/tests/kits/storage/DataIOTest.h new file mode 100644 index 0000000000..235baec492 --- /dev/null +++ b/src/tests/kits/storage/DataIOTest.h @@ -0,0 +1,23 @@ +// DataIOTest.h + +#ifndef __sk_data_io_test_h__ +#define __sk_data_io_test_h__ + +#include +#include + +#include +#include + +#include "BasicTest.h" + +class DataIOTest : public BasicTest +{ +public: + static CppUnit::Test* Suite(); + + void BufferedDataIOTest(); +}; + + +#endif // __sk_data_io_test_h__ diff --git a/src/tests/kits/storage/Jamfile b/src/tests/kits/storage/Jamfile index b2d0d09fc1..59a146f5e6 100644 --- a/src/tests/kits/storage/Jamfile +++ b/src/tests/kits/storage/Jamfile @@ -8,6 +8,7 @@ UnitTestLib libstoragetest.so : StorageKitTestAddon.cpp AppFileInfoTest.cpp BasicTest.cpp + DataIOTest.cpp DirectoryTest.cpp EntryTest.cpp FindDirectoryTest.cpp diff --git a/src/tests/kits/storage/StorageKitTestAddon.cpp b/src/tests/kits/storage/StorageKitTestAddon.cpp index 17f9417707..4cc8bbe0e3 100644 --- a/src/tests/kits/storage/StorageKitTestAddon.cpp +++ b/src/tests/kits/storage/StorageKitTestAddon.cpp @@ -4,6 +4,7 @@ // ##### Include headers for your tests here ##### #include "AppFileInfoTest.h" #include "DirectoryTest.h" +#include "DataIOTest.h" #include "EntryTest.h" #include "FileTest.h" #include "FindDirectoryTest.h" @@ -24,6 +25,7 @@ BTestSuite* getTestSuite() { // ##### Add test suites here ##### suite->addTest("BAppFileInfo", AppFileInfoTest::Suite()); suite->addTest("BDirectory", DirectoryTest::Suite()); + suite->addTest("BDataIO", DataIOTest::Suite()); suite->addTest("BEntry", EntryTest::Suite()); suite->addTest("BFile", FileTest::Suite()); #if 0