From a6b9de9a59bbe92263ac7226e175ff0ba897da79 Mon Sep 17 00:00:00 2001 From: Gary Talent Date: Wed, 6 Jun 2018 23:30:57 -0500 Subject: [PATCH] [ox] Fix OxFS 2 Directory test --- .../src/ox/fs/filestore/filestoretemplate.hpp | 10 ++- deps/ox/src/ox/fs/filesystem/pathiterator.hpp | 7 +- deps/ox/src/ox/fs/filesystem2/directory.hpp | 75 ++++++++++++------- deps/ox/src/ox/fs/test/tests.cpp | 8 +- deps/ox/src/ox/mc/write.hpp | 8 +- deps/ox/src/ox/ptrarith/nodebuffer.hpp | 47 +++++++----- deps/ox/src/ox/ptrarith/ptr.hpp | 8 +- deps/ox/src/ox/std/string.hpp | 4 +- deps/ox/src/ox/std/strops.hpp | 16 ++++ deps/ox/src/ox/trace/trace.cpp | 11 +-- deps/ox/src/ox/trace/trace.hpp | 15 ++-- 11 files changed, 131 insertions(+), 78 deletions(-) diff --git a/deps/ox/src/ox/fs/filestore/filestoretemplate.hpp b/deps/ox/src/ox/fs/filestore/filestoretemplate.hpp index d8aafc8b..069b5dcf 100644 --- a/deps/ox/src/ox/fs/filestore/filestoretemplate.hpp +++ b/deps/ox/src/ox/fs/filestore/filestoretemplate.hpp @@ -208,7 +208,12 @@ Error FileStoreTemplate::write(InodeId_t id, void *data, FsSize_t dataSi if (canWrite(existing, dataSize)) { // delete the old node if it exists if (existing.valid()) { - m_buffer->free(existing); + oxTrace("ox::fs::FileStoreTemplate::write") << "Freeing old version of inode found at offset:" << existing.offset(); + auto err = m_buffer->free(existing); + if (err) { + oxTrace("ox::fs::FileStoreTemplate::write::fail") << "Free of old version of inode failed"; + return err; + } existing = nullptr; } @@ -220,7 +225,6 @@ Error FileStoreTemplate::write(InodeId_t id, void *data, FsSize_t dataSi dest = m_buffer->malloc(dataSize); } if (dest.valid()) { - new (dest) FileStoreItem(dataSize); dest->id = id; dest->fileType = fileType; auto destData = m_buffer->template dataOf(dest); @@ -507,7 +511,7 @@ typename FileStoreTemplate::ItemPtr FileStoreTemplate::find(Item return find(m_buffer->ptr(item->right), id, depth + 1); } else if (id < item->id) { return find(m_buffer->ptr(item->left), id, depth + 1); - } else { + } else if (id == item->id) { return item; } } else { diff --git a/deps/ox/src/ox/fs/filesystem/pathiterator.hpp b/deps/ox/src/ox/fs/filesystem/pathiterator.hpp index f68844cb..c1359874 100644 --- a/deps/ox/src/ox/fs/filesystem/pathiterator.hpp +++ b/deps/ox/src/ox/fs/filesystem/pathiterator.hpp @@ -12,7 +12,8 @@ namespace ox { -constexpr auto MaxFileNameLength = 255; +constexpr std::size_t MaxFileNameLength = 255; +using FileName = BString; class PathIterator { private: @@ -48,12 +49,12 @@ class PathIterator { /** * @return 0 if no error */ - Error next(BString *fileName); + Error next(FileName *fileName); /** * @return 0 if no error */ - Error get(BString *fileName); + Error get(FileName *fileName); /** * @return 0 if no error diff --git a/deps/ox/src/ox/fs/filesystem2/directory.hpp b/deps/ox/src/ox/fs/filesystem2/directory.hpp index 19aca689..dd6ac304 100644 --- a/deps/ox/src/ox/fs/filesystem2/directory.hpp +++ b/deps/ox/src/ox/fs/filesystem2/directory.hpp @@ -23,6 +23,11 @@ struct __attribute__((packed)) DirectoryEntry { // DirectoryEntry fields LittleEndian inode = 0; char name[MaxFileNameLength]; + + static constexpr std::size_t spaceNeeded(std::size_t chars) { + return offsetof(DirectoryEntryData, name) + chars; + } + }; // NodeBuffer fields @@ -37,16 +42,23 @@ struct __attribute__((packed)) DirectoryEntry { DirectoryEntry() = default; DirectoryEntry(InodeId_t inode, const char *name, InodeId_t bufferSize) { + init(inode, name, bufferSize); + } + + Error init(InodeId_t inode, const char *name, InodeId_t bufferSize) { + m_bufferSize = bufferSize; auto d = data(); if (d.valid()) { d->inode = inode; - ox_strncpy(d->name, name, MaxFileNameLength); - m_bufferSize = bufferSize; + ox_strncpy(d->name, name, ox::min(bufferSize, static_cast(MaxFileNameLength))); + return OxError(0); } + return OxError(1); } ptrarith::Ptr data() { - return ptrarith::Ptr(this, this->fullSize(), sizeof(*this), this->size()); + oxTrace("ox::ptrarith::DirectoryEntry::data") << this->fullSize() << sizeof(*this) << this->size(); + return ptrarith::Ptr(this, this->fullSize(), sizeof(*this), this->size(), this->size()); } /** @@ -57,14 +69,14 @@ struct __attribute__((packed)) DirectoryEntry { } InodeId_t size() const { - return fullSize() - offsetof(DirectoryEntryData, inode); + return fullSize() - sizeof(*this); } void setSize(InodeId_t) { // ignore set value } - static std::size_t spaceNeeded(std::size_t chars) { + static constexpr std::size_t spaceNeeded(std::size_t chars) { return sizeof(DirectoryEntry) + offsetof(DirectoryEntryData, name) + chars; } @@ -89,11 +101,11 @@ class Directory { */ Error init() noexcept; - Error write(PathIterator it, InodeId_t inode) noexcept; + Error write(PathIterator it, InodeId_t inode, FileName *nameBuff = nullptr) noexcept; Error rm(PathIterator it) noexcept; - ValErr find(const BString &name) const noexcept; + ValErr find(const FileName &name) const noexcept; }; @@ -122,45 +134,53 @@ Error Directory::init() noexcept { } template -Error Directory::write(PathIterator path, InodeId_t inode) noexcept { +Error Directory::write(PathIterator path, InodeId_t inode, FileName *nameBuff) noexcept { InodeId_t nextChild = 0; + + // reuse nameBuff if it has already been allocated, as it is a rather large variable + if (nameBuff == nullptr) { + nameBuff = reinterpret_cast(ox_alloca(sizeof(FileName))); + } + auto name = nameBuff; + if ((path + 1).hasNext()) { oxTrace("ox::fs::Directory::write") << "Attempting to write to next sub-Directory"; - // read the name and pop it off the stack as soon as possible to help - // avoid a stack overflow in the recursive calls - { - BString name; - path.get(&name); - nextChild = find(name); - } + path.get(name); + nextChild = find(*name); // It's important for the recursive call to be outside the block where name - // lived. This avoids allocation several BStrings on the + // lived. This avoids allocation several FileNames on the // stack at once, while also avoiding the use of heap memory. if (nextChild) { - return Directory(m_fs, nextChild).write(path + 1, inode); + // reuse name because it is a rather large variable and will not be used again + // be attentive that this remains true + name = nullptr; + return Directory(m_fs, nextChild).write(path + 1, inode, nameBuff); } } else { // insert the new entry on this directory // get the name - BString name; - path.next(&name); + path.next(name); + + oxTrace("ox::fs::Directory::write") << "Attempting to write Directory to FileStore"; // find existing version of directory auto old = m_fs->read(m_inodeId); if (old.valid()) { - const auto newSize = m_size + DirectoryEntry::spaceNeeded(name.size()); + const auto entrySize = DirectoryEntry::spaceNeeded(name->len()); + const auto entryDataSize =DirectoryEntry::DirectoryEntryData::spaceNeeded(name->len()); + const auto newSize = m_size + entrySize; auto cpy = ox_malloca(newSize, Buffer, old); if (cpy != nullptr) { // TODO: look for old version of this entry and delete it cpy->setSize(newSize); - auto val = cpy->malloc(name.size()); + auto val = cpy->malloc(entryDataSize); if (val.valid()) { oxTrace("ox::fs::Directory::write") << "Attempting to write Directory to FileStore"; - new (val) DirectoryEntry(inode, name.data(), newSize); + val->init(inode, name->data(), entrySize); return m_fs->write(m_inodeId, cpy, cpy->size()); } else { oxTrace("ox::fs::Directory::write::fail") << "Could not allocate memory for new directory entry"; @@ -181,16 +201,19 @@ Error Directory::rm(PathIterator) noexcept { } template -ValErr Directory::find(const BString &name) const noexcept { +ValErr Directory::find(const FileName &name) const noexcept { oxTrace("ox::fs::Directory::find") << name.c_str(); auto buff = m_fs->read(m_inodeId).template to(); if (buff.valid()) { oxTrace("ox::fs::Directory::find") << "Found directory buffer."; for (auto i = buff->iterator(); i.valid(); i.next()) { auto data = i->data(); - oxTrace("ox::fs::Directory::find::name") << name.c_str(); - if (data.valid() && data->name == name.c_str()) { - return static_cast(data->inode); + if (data.valid()) { + if (ox_strncmp(data->name, name.c_str(), name.len()) == 0) { + return static_cast(data->inode); + } + } else { + oxTrace("ox::fs::Directory::find") << "INVALID DIRECTORY ENTRY"; } } oxTrace("ox::fs::Directory::find::fail"); diff --git a/deps/ox/src/ox/fs/test/tests.cpp b/deps/ox/src/ox/fs/test/tests.cpp index 6129a01d..6b00e4aa 100644 --- a/deps/ox/src/ox/fs/test/tests.cpp +++ b/deps/ox/src/ox/fs/test/tests.cpp @@ -395,11 +395,11 @@ map tests = { oxAssert(dir->find("file1").error, "Could not find file1"); oxAssert(dir->find("file1") == 1, "Could not find file1"); - //oxTrace("ox::fs::test::Directory") << "write 2"; - //oxAssert(dir->write("/file3", 3) == 0, "Directory write of file3 failed"); + oxTrace("ox::fs::test::Directory") << "write 2"; + oxAssert(dir->write("/file3", 3) == 0, "Directory write of file3 failed"); - //oxTrace("ox::fs::test::Directory") << "write 3"; - //oxAssert(dir->write("/file2", 2) == 0, "Directory write of file2 failed"); + oxTrace("ox::fs::test::Directory") << "write 3"; + oxAssert(dir->write("/file2", 2) == 0, "Directory write of file2 failed"); return 0; } diff --git a/deps/ox/src/ox/mc/write.hpp b/deps/ox/src/ox/mc/write.hpp index 9eb81ace..6f709539 100644 --- a/deps/ox/src/ox/mc/write.hpp +++ b/deps/ox/src/ox/mc/write.hpp @@ -74,13 +74,13 @@ int MetalClawWriter::op(const char*, ox::BString *val) { if (val->len()) { // write the length typedef uint32_t StringLength; - if (m_buffIt + sizeof(StringLength) + val->size() < m_buffLen) { - *reinterpret_cast*>(&m_buff[m_buffIt]) = static_cast(val->size()); + if (m_buffIt + sizeof(StringLength) + val->bytes() < m_buffLen) { + *reinterpret_cast*>(&m_buff[m_buffIt]) = static_cast(val->bytes()); m_buffIt += sizeof(StringLength); // write the string - ox_memcpy(&m_buff[m_buffIt], val, val->size()); - m_buffIt += val->size(); + ox_memcpy(&m_buff[m_buffIt], val, val->bytes()); + m_buffIt += val->bytes(); fieldSet = true; } else { err = MC_BUFFENDED; diff --git a/deps/ox/src/ox/ptrarith/nodebuffer.hpp b/deps/ox/src/ox/ptrarith/nodebuffer.hpp index 56bad1d2..dc253dce 100644 --- a/deps/ox/src/ox/ptrarith/nodebuffer.hpp +++ b/deps/ox/src/ox/ptrarith/nodebuffer.hpp @@ -43,6 +43,10 @@ class __attribute__((packed)) NodeBuffer { return m_current; } + Item *get() { + return m_current; + } + operator Item*() { return m_current; } @@ -55,7 +59,7 @@ class __attribute__((packed)) NodeBuffer { return m_current; } - bool valid() { + bool valid() const noexcept { return m_current.valid(); } @@ -70,7 +74,11 @@ class __attribute__((packed)) NodeBuffer { void next() { oxTrace("ox::ptrarith::NodeBuffer::Iterator::next") << m_it++; - m_current = m_buffer->next(m_current); + if (hasNext()) { + m_current = m_buffer->next(m_current); + } else { + m_current = nullptr; + } } }; @@ -107,7 +115,7 @@ class __attribute__((packed)) NodeBuffer { ItemPtr malloc(size_t size); - void free(ItemPtr item); + Error free(ItemPtr item); bool valid(size_t maxSize); @@ -192,11 +200,10 @@ typename NodeBuffer::ItemPtr NodeBuffer::next(Item * template typename NodeBuffer::ItemPtr NodeBuffer::ptr(size_t itemOffset) { // make sure this can be read as an Item, and then use Item::size for the size - auto itemSpace = m_header.size - itemOffset; + std::size_t itemSpace = m_header.size - itemOffset; auto item = reinterpret_cast(reinterpret_cast(this) + itemOffset); if (itemOffset >= sizeof(Header) && - itemOffset < m_header.size - sizeof(Item) && - itemSpace >= static_cast(sizeof(Item)) && + itemSpace >= sizeof(Item) && itemSpace >= item->fullSize()) { return ItemPtr(this, m_header.size, itemOffset, item->fullSize()); } else { @@ -207,6 +214,7 @@ typename NodeBuffer::ItemPtr NodeBuffer::ptr(size_t template typename NodeBuffer::ItemPtr NodeBuffer::malloc(size_t size) { + oxTrace("ox::ptrarith::NodeBuffer::malloc") << "Size:" << size; size_t fullSize = size + sizeof(Item); if (m_header.size - m_header.bytesUsed >= fullSize) { auto last = lastItem(); @@ -216,7 +224,7 @@ typename NodeBuffer::ItemPtr NodeBuffer::malloc(size } else { // there is no first item, so this may be the first item if (!m_header.firstItem) { - oxTrace("ox::fs::NodeBuffer::malloc") << "No first item, initializing."; + oxTrace("ox::ptrarith::NodeBuffer::malloc") << "No first item, initializing."; m_header.firstItem = sizeof(m_header); addr = m_header.firstItem; } @@ -231,7 +239,7 @@ typename NodeBuffer::ItemPtr NodeBuffer::malloc(size if (first.valid()) { first->prev = out.offset(); } else { - oxTrace("ox::fs::NodeBuffer::malloc::fail") << "NodeBuffer malloc failed due to invalid first element pointer."; + oxTrace("ox::ptrarith::NodeBuffer::malloc::fail") << "NodeBuffer malloc failed due to invalid first element pointer."; return nullptr; } @@ -240,34 +248,37 @@ typename NodeBuffer::ItemPtr NodeBuffer::malloc(size if (last.valid()) { last->next = out.offset(); } else { - oxTrace("ox::fs::NodeBuffer::malloc::fail") << "NodeBuffer malloc failed due to invalid last element pointer."; + oxTrace("ox::ptrarith::NodeBuffer::malloc::fail") << "NodeBuffer malloc failed due to invalid last element pointer."; return nullptr; } m_header.bytesUsed += out.size(); } else { - oxTrace("ox::fs::NodeBuffer::malloc::fail") << "Unknown"; + oxTrace("ox::ptrarith::NodeBuffer::malloc::fail") << "Unknown"; } return out; } - oxTrace("ox::fs::NodeBuffer::malloc::fail") << "Insufficient space:" << fullSize << "needed," << available() << "available"; + oxTrace("ox::ptrarith::NodeBuffer::malloc::fail") << "Insufficient space:" << fullSize << "needed," << available() << "available"; return nullptr; } template -void NodeBuffer::free(ItemPtr item) { +Error NodeBuffer::free(ItemPtr item) { auto prev = this->prev(item); auto next = this->next(item); - if (prev.valid()) { + if (prev.valid() && next.valid()) { prev->next = next.offset(); - } else { - oxTrace("ox::fs::NodeBuffer::free::fail") << "NodeBuffer free failed due to invalid prev element pointer."; - } - if (next.valid()) { next->prev = prev.offset(); } else { - oxTrace("ox::fs::NodeBuffer::free::fail") << "NodeBuffer free failed due to invalid next element pointer."; + if (!prev.valid()) { + oxTrace("ox::ptrarith::NodeBuffer::free::fail") << "NodeBuffer free failed due to invalid prev element pointer:" << prev.offset(); + } + if (!next.valid()) { + oxTrace("ox::ptrarith::NodeBuffer::free::fail") << "NodeBuffer free failed due to invalid next element pointer:" << next.offset(); + } + return OxError(1); } m_header.bytesUsed -= item.size(); + return OxError(0); } template diff --git a/deps/ox/src/ox/ptrarith/ptr.hpp b/deps/ox/src/ox/ptrarith/ptr.hpp index a39e640e..3bbae10c 100644 --- a/deps/ox/src/ox/ptrarith/ptr.hpp +++ b/deps/ox/src/ox/ptrarith/ptr.hpp @@ -29,7 +29,7 @@ class Ptr { inline Ptr(std::nullptr_t); - inline Ptr(void *dataStart, size_t dataSize, size_t itemStart, size_t itemSize = sizeof(T)); + inline Ptr(void *dataStart, size_t dataSize, size_t itemStart, size_t itemSize = sizeof(T), size_t itemTypeSize = sizeof(T)); inline bool valid() const; @@ -83,12 +83,12 @@ inline Ptr::Ptr(std::nullptr_t) { } template -inline Ptr::Ptr(void *dataStart, size_t dataSize, size_t itemStart, size_t itemSize) { +inline Ptr::Ptr(void *dataStart, size_t dataSize, size_t itemStart, size_t itemSize, size_t itemTypeSize) { // do some sanity checks before assuming this is valid - if (itemSize >= sizeof(T) && + if (itemSize >= itemTypeSize && dataStart && itemStart >= minOffset && - static_cast(itemStart + itemSize) <= dataSize) { + itemStart + itemSize <= dataSize) { m_dataStart = reinterpret_cast(dataStart); m_dataSize = dataSize; m_itemOffset = itemStart; diff --git a/deps/ox/src/ox/std/string.hpp b/deps/ox/src/ox/std/string.hpp index 15fb1782..8728cacd 100644 --- a/deps/ox/src/ox/std/string.hpp +++ b/deps/ox/src/ox/std/string.hpp @@ -51,7 +51,7 @@ class BString { /** * Returns the number of bytes used for this string. */ - std::size_t size() const noexcept; + std::size_t bytes() const noexcept; /** * Returns the capacity of bytes for this string. @@ -162,7 +162,7 @@ std::size_t BString::len() const noexcept { } template -std::size_t BString::size() const noexcept { +std::size_t BString::bytes() const noexcept { std::size_t i; for (i = 0; i < buffLen && m_buff[i]; i++); return i + 1; // add one for null terminator diff --git a/deps/ox/src/ox/std/strops.hpp b/deps/ox/src/ox/std/strops.hpp index 6cf146bb..78f7339d 100644 --- a/deps/ox/src/ox/std/strops.hpp +++ b/deps/ox/src/ox/std/strops.hpp @@ -53,6 +53,22 @@ constexpr int ox_strcmp(const char *str1, const char *str2) noexcept { return retval; } +constexpr int ox_strncmp(const char *str1, const char *str2, std::size_t len) noexcept { + auto retval = 0; + std::size_t i = 0; + while (i < len && (str1[i] || str2[i])) { + if (str1[i] < str2[i]) { + retval = -1; + break; + } else if (str1[i] > str2[i]) { + retval = 1; + break; + } + i++; + } + return retval; +} + constexpr const char *ox_strchr(const char *str, int character, std::size_t maxLen = 0xFFFFFFFF) noexcept { for (std::size_t i = 0; i <= maxLen; i++) { if (str[i] == character) { diff --git a/deps/ox/src/ox/trace/trace.cpp b/deps/ox/src/ox/trace/trace.cpp index 1d8ab763..8ab335e4 100644 --- a/deps/ox/src/ox/trace/trace.cpp +++ b/deps/ox/src/ox/trace/trace.cpp @@ -41,20 +41,13 @@ StdOutStream::StdOutStream(const char *file, int line, const char *ch, const cha StdOutStream::~StdOutStream() { #if defined(OX_USE_STDLIB) - std::cout << std::setw(50) << std::left << m_msg.ch.c_str() << '|'; - std::cout << std::setw(60) << std::left << m_msg.msg.c_str() << '|'; + std::cout << std::setw(53) << std::left << m_msg.ch.c_str() << '|'; + std::cout << std::setw(65) << std::left << m_msg.msg.c_str() << '|'; std::cout << " " << m_msg.file.c_str() << ':' << m_msg.line << "\n"; #endif } -NullStream::NullStream(const char*, int, const char*, const char*) { -} - -NullStream::~NullStream() { -} - - void logError(const char *file, int line, Error err) { if (err) { ErrorInfo ei(err); diff --git a/deps/ox/src/ox/trace/trace.hpp b/deps/ox/src/ox/trace/trace.hpp index 58316a59..76fb8e4f 100644 --- a/deps/ox/src/ox/trace/trace.hpp +++ b/deps/ox/src/ox/trace/trace.hpp @@ -66,7 +66,7 @@ class StdOutStream { ~StdOutStream(); template - inline StdOutStream &operator<<(const T &v) { + constexpr inline StdOutStream &operator<<(const T &v) { m_msg.msg += " "; m_msg.msg += v; return *this; @@ -78,20 +78,25 @@ class StdOutStream { class NullStream { public: - NullStream() = default; + constexpr NullStream() = default; - NullStream(const char *file, int line, const char *ch, const char *msg = ""); + constexpr NullStream(const char*, int, const char*, const char* = "") { + } - ~NullStream(); + ~NullStream() = default; template - inline NullStream &operator<<(const T&) { + constexpr inline NullStream &operator<<(const T&) { return *this; } }; +#ifdef DEBUG using TraceStream = StdOutStream; +#else +using TraceStream = NullStream; +#endif void logError(const char *file, int line, Error err);