pnor_partition: Refactor to enforce stronger boundaries for abstractions

The RORequest and RWRequest classes did not provide a clear abstraction
over the operation of populating a window or partition associated with a
CREATE_{READ,WRITE}_WINDOW request. The role of the classes was to find
the partition for the provided offset, locate and then open its backing
file.

However, the file-descriptor for the backing file was exposed outside of
the class, as was the FFS partition struct, both of which were managed
_internal_ to the class. Thus the classes provided no encapsulation of
state and awkwardly split the tasks of managing and utilising the
resources between the callee and caller.

This commit inverts the behaviour in a fulfil() method handles the
mechanics of locating, opening, manipulating and closing the backing
file, requiring nothing of the caller. The pnor_partition reference is
managed entirely inside the Request class, derived from the offset
passed to the constructor.

Unifying the mechanics into fulfil() results in a decent reduction in
lines of code at the expense of some cyclomatic complexity. fulfil() is
publicly exposed via read() and write() wrappers on the class, and the
RORequest and RWRequest classes are removed as a result.

Change-Id: Ie587ed31f1a6db97bcb490dbcc2e27ece0b1f82c
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

3 files changed, 152 insertions, 263 deletions

diff --git a/mboxd_flash_virtual.cpp b/mboxd_flash_virtual.cpp
index 1561396..ffe75c6 100644
--- a/mboxd_flash_virtual.cpp
+++ b/mboxd_flash_virtual.cpp
@@ -29,6 +29,7 @@ extern "C" {
 #include <stdexcept>
 
 namespace err = sdbusplus::xyz::openbmc_project::Common::Error;
+namespace fs = std::experimental::filesystem;
 namespace vpnor = openpower::virtual_pnor;
 
 /** @brief unique_ptr functor to release a char* reference. */
@@ -119,10 +120,6 @@ int erase_flash(struct mbox_context* context, uint32_t offset, uint32_t count)
 int64_t copy_flash(struct mbox_context* context, uint32_t offset, void* mem,
                    uint32_t size)
 {
-    using namespace phosphor::logging;
-    using namespace sdbusplus::xyz::openbmc_project::Common::Error;
-    using namespace std::string_literals;
-
     vpnor::partition::Table* table;
     int rc = size;
 
@@ -152,30 +149,8 @@ int64_t copy_flash(struct mbox_context* context, uint32_t offset, void* mem,
 
     try
     {
-        openpower::virtual_pnor::RORequest roRequest;
-        auto partitionInfo = roRequest.getPartitionInfo(context, offset);
-
-        auto mapped_mem = mmap(NULL, partitionInfo->data.actual, PROT_READ,
-                               MAP_PRIVATE, roRequest.fd(), 0);
-
-        if (mapped_mem == MAP_FAILED)
-        {
-            MSG_ERR("Failed to map partition=%s:Error=%s\n",
-                    partitionInfo->data.name, strerror(errno));
-
-            elog<InternalFailure>();
-        }
-
-        // if the asked offset + no of bytes to read is greater
-        // then size of the partition file then throw error.
-
-        uint32_t baseOffset = partitionInfo->data.base
-                              << context->block_size_shift;
-        // copy to the reserved memory area
-        auto diffOffset = offset - baseOffset;
-        rc = std::min(partitionInfo->data.actual - diffOffset, size);
-        memcpy(mem, (char*)mapped_mem + diffOffset, rc);
-        munmap(mapped_mem, partitionInfo->data.actual);
+        vpnor::Request req(context, offset);
+        rc = req.read(mem, size);
     }
     catch (vpnor::UnmappedOffset& e)
     {
@@ -214,51 +189,28 @@ int64_t copy_flash(struct mbox_context* context, uint32_t offset, void* mem,
 int write_flash(struct mbox_context* context, uint32_t offset, void* buf,
                 uint32_t count)
 {
-    using namespace phosphor::logging;
-    using namespace sdbusplus::xyz::openbmc_project::Common::Error;
-    using namespace std::string_literals;
 
-    int rc = 0;
-    MSG_DBG("Write flash @ 0x%.8x for 0x%.8x from %p\n", offset, count, buf);
-    try
+    if (!(context && context->vpnor && context->vpnor->table))
     {
-        openpower::virtual_pnor::RWRequest rwRequest;
-        auto partitionInfo = rwRequest.getPartitionInfo(context, offset);
-
-        auto mapped_mem =
-            mmap(NULL, partitionInfo->data.actual, PROT_READ | PROT_WRITE,
-                 MAP_SHARED, rwRequest.fd(), 0);
-        if (mapped_mem == MAP_FAILED)
-        {
-            MSG_ERR("Failed to map partition=%s:Error=%s\n",
-                    partitionInfo->data.name, strerror(errno));
-
-            elog<InternalFailure>();
-        }
-        // copy to the mapped memory.
+        MSG_ERR("Trying to write data with uninitialised context!\n");
+        return -MBOX_R_SYSTEM_ERROR;
+    }
 
-        uint32_t baseOffset = partitionInfo->data.base
-                              << context->block_size_shift;
+    vpnor::partition::Table* table = context->vpnor->table;
 
-        // if the asked offset + no of bytes to write is greater
-        // then size of the partition file then throw error.
-        //
-        // FIXME: Don't use .actual, use  (.size << ctx->block_size_shift),
-        // otherwise we can't grow the size of the data to fill the partition
-        if ((offset + count) > (baseOffset + partitionInfo->data.actual))
+    try
+    {
+        const struct pnor_partition& part = table->partition(offset);
+        if (part.data.user.data[1] & PARTITION_READONLY)
         {
-            munmap(mapped_mem, partitionInfo->data.actual);
-            std::stringstream err;
-            err << "Write extends beyond the partition length " << std::hex
-                << partitionInfo->data.actual;
-            throw vpnor::OutOfBoundsOffset(err.str());
+            /* FIXME: This should be done on CREATE_WRITE_WINDOW, not here */
+            return -MBOX_R_WRITE_ERROR;
         }
 
-        auto diffOffset = offset - baseOffset;
-        memcpy((char*)mapped_mem + diffOffset, buf, count);
-        munmap(mapped_mem, partitionInfo->data.actual);
-
-        set_flash_bytemap(context, offset, count, FLASH_DIRTY);
+        MSG_DBG("Write flash @ 0x%.8x for 0x%.8x from %p\n", offset, count,
+                buf);
+        vpnor::Request req(context, offset);
+        req.write(buf, count);
     }
     catch (vpnor::UnmappedOffset& e)
     {
@@ -279,5 +231,5 @@ int write_flash(struct mbox_context* context, uint32_t offset, void* buf,
         phosphor::logging::commit<err::InternalFailure>();
         return -MBOX_R_SYSTEM_ERROR;
     }
-    return rc;
+    return 0;
 }
diff --git a/pnor_partition.cpp b/pnor_partition.cpp
index cdb9633..390c4d5 100644
--- a/pnor_partition.cpp
+++ b/pnor_partition.cpp
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: Apache-2.0
 // Copyright (C) 2018 IBM Corp.
 #include "pnor_partition.hpp"
+#include "pnor_partition_table.hpp"
 #include "config.h"
 #include "mboxd_flash.h"
 #include "mboxd_pnor_partition_table.h"
@@ -8,6 +9,7 @@
 #include <phosphor-logging/log.hpp>
 #include <phosphor-logging/elog-errors.hpp>
 
+#include <assert.h>
 #include <stdint.h>
 #include <stdlib.h>
 #include <syslog.h>
@@ -26,185 +28,126 @@ namespace openpower
 namespace virtual_pnor
 {
 
-using namespace phosphor::logging;
-using namespace sdbusplus::xyz::openbmc_project::Common::Error;
-using namespace std::string_literals;
+namespace fs = std::experimental::filesystem;
 
-ReturnCode Request::open(const std::string& path, int mode)
+fs::path Request::getPartitionFilePath(int flags)
 {
-    if (mode == O_RDWR && partition->data.user.data[1] & PARTITION_READONLY)
-    {
-        MSG_ERR("Can't open RO partition '%s' for write\n", path.c_str());
-        return ReturnCode::PARTITION_READ_ONLY;
-    }
-
-    fs::path partitionFilePath = path;
-
-    if (!fs::exists(partitionFilePath))
-    {
-        return ReturnCode::FILE_NOT_FOUND;
-    }
-
-    auto descriptor = ::open(partitionFilePath.c_str(), mode);
-    if (descriptor < 0)
-    {
-        return ReturnCode::FILE_OPEN_FAILURE;
-    }
-
-    fd.set(descriptor);
-    descriptor = -1;
-
-    return ReturnCode::SUCCESS;
-}
-
-std::string Request::getPartitionFilePath(struct mbox_context* context,
-                                          uint32_t offset)
-{
-    partition = vpnor_get_partition(context, offset);
-    if (!partition)
-    {
-        MSG_ERR("Couldn't get the partition info for offset 0x%" PRIx32 "\n",
-                offset);
-        elog<InternalFailure>();
-    }
-
-    fs::path partitionFilePath;
-
     // Check if partition exists in patch location
-    partitionFilePath = context->paths.patch_loc;
-    partitionFilePath /= partition->data.name;
-    if (fs::is_regular_file(partitionFilePath))
+    auto dst = fs::path(ctx->paths.patch_loc) / partition.data.name;
+    if (fs::is_regular_file(dst))
     {
-        return partitionFilePath.string();
+        return dst;
     }
 
-    switch (partition->data.user.data[1] &
+    switch (partition.data.user.data[1] &
             (PARTITION_PRESERVED | PARTITION_READONLY))
     {
         case PARTITION_PRESERVED:
-            partitionFilePath = context->paths.prsv_loc;
+            dst = ctx->paths.prsv_loc;
             break;
 
         case PARTITION_READONLY:
-            partitionFilePath = context->paths.ro_loc;
+            dst = ctx->paths.ro_loc;
             break;
 
         default:
-            partitionFilePath = context->paths.rw_loc;
+            dst = ctx->paths.rw_loc;
     }
-    partitionFilePath /= partition->data.name;
-    return partitionFilePath.string();
-}
+    dst /= partition.data.name;
 
-const pnor_partition* RORequest::getPartitionInfo(struct mbox_context* context,
-                                                  uint32_t offset)
-{
-    std::string path = getPartitionFilePath(context, offset);
-    ReturnCode rc = Request::open(path, O_RDONLY);
-    if (rc == ReturnCode::SUCCESS)
+    if (fs::exists(dst))
     {
-        return partition;
-    }
-    // not interested in any other error except FILE_NOT_FOUND
-    if (rc != ReturnCode::FILE_NOT_FOUND)
-    {
-        MSG_ERR(
-            "RORequest: Error opening partition file '%s' (offset 0x%" PRIx32
-            "): %u\n",
-            path.c_str(), offset, static_cast<uint8_t>(rc));
-        elog<InternalFailure>();
+        return dst;
     }
 
-    // if the offset lies in read only partition then throw error.
-    if (partition->data.user.data[1] & PARTITION_READONLY)
+    if (flags == O_RDONLY)
     {
-        MSG_ERR("RORequest: Requested offset 0x%" PRIx32
-                " is in a read-only partition (%s)\n",
-                offset, path.c_str());
-        elog<InternalFailure>();
+        dst = fs::path(ctx->paths.ro_loc) / partition.data.name;
+        assert(fs::exists(dst));
+        return dst;
     }
 
-    // we don't get the file in the respective partition(RW/PSRV)
-    // try to open it from RO location.
+    assert(flags == O_RDWR);
+    auto src = fs::path(ctx->paths.ro_loc) / partition.data.name;
+    assert(fs::exists(src));
 
-    fs::path partitionFilePath = context->paths.ro_loc;
-    partitionFilePath /= partition->data.name;
+    MSG_DBG("RWRequest: Didn't find '%s' under '%s', copying from '%s'\n",
+            partition.data.name, dst.c_str(), src.c_str());
 
-    rc = Request::open(partitionFilePath, O_RDONLY);
-    if (rc != ReturnCode::SUCCESS)
+    dst = ctx->paths.rw_loc;
+    if (partition.data.user.data[1] & PARTITION_PRESERVED)
     {
-        MSG_ERR("RORequest: Failed to open partition file '%s' at RO fallback "
-                "(offset 0x%" PRIx32 "): %u\n",
-                partitionFilePath.c_str(), offset, static_cast<uint8_t>(rc));
-        elog<InternalFailure>();
+        dst = ctx->paths.prsv_loc;
     }
 
-    return partition;
+    dst /= partition.data.name;
+    fs::copy_file(src, dst);
+
+    return dst;
 }
 
-const pnor_partition* RWRequest::getPartitionInfo(struct mbox_context* context,
-                                                  uint32_t offset)
+ssize_t Request::fulfil(void *buf, size_t len, int flags)
 {
-    std::string path = getPartitionFilePath(context, offset);
-    std::error_code ec;
-
-    ReturnCode rc = Request::open(path, O_RDWR);
-    if (rc == ReturnCode::SUCCESS)
-    {
-        return partition;
-    }
-    // not interested in any other error except FILE_NOT_FOUND
-    if (rc != ReturnCode::FILE_NOT_FOUND)
+    if (!(flags == O_RDONLY || flags == O_RDWR))
     {
-        MSG_ERR(
-            "RWRequest: Error opening partition file '%s' (offset 0x%" PRIx32
-            "): %d\n",
-            path.c_str(), offset, static_cast<uint8_t>(rc));
-        elog<InternalFailure>();
+        std::stringstream err;
+        err << "Require O_RDONLY (0x" << std::hex << O_RDONLY << " or O_RDWR "
+            << std::hex << O_RDWR << " for flags, got: 0x" << std::hex << flags;
+        throw std::invalid_argument(err.str());
     }
 
-    // if the file is not available in the respective(RW/PSRV) partition
-    // then copy the file from RO to the respective(RW/PSRV) partition
-    // and open it for writing.
+    fs::path path = getPartitionFilePath(flags);
 
-    fs::path fromPath = context->paths.ro_loc;
-    fromPath /= partition->data.name;
-    if (!fs::exists(fromPath))
+    int fd = ::open(path.c_str(), flags);
+    if (fd == -1)
     {
-        MSG_ERR("RWRequest: Partition '%s' for offset 0x%" PRIx32
-                " not found in RO directory '%s'\n",
-                partition->data.name, offset, context->paths.ro_loc);
-        elog<InternalFailure>();
+        MSG_ERR("Failed to open backing file at '%s': %d\n",
+                path.c_str(), errno);
+        throw std::system_error(errno, std::system_category());
     }
-    // copy the file from ro to respective partition
-    fs::path toPath = context->paths.rw_loc;
 
-    if (partition->data.user.data[1] & PARTITION_PRESERVED)
+    int mprot = PROT_READ | ((flags == O_RDWR) ? PROT_WRITE : 0);
+    auto map = mmap(NULL, partition.data.actual, mprot, MAP_SHARED, fd, 0);
+    if (map == MAP_FAILED)
     {
-        toPath = context->paths.prsv_loc;
+        close(fd);
+        MSG_ERR("Failed to map backing file '%s' for %d bytes: %d\n",
+                path.c_str(), partition.data.actual, errno);
+        throw std::system_error(errno, std::system_category());
     }
 
-    MSG_DBG("RWRequest: Didn't find '%s' under '%s', copying from '%s'\n",
-            partition->data.name, toPath.c_str(), fromPath.c_str());
-
-    toPath /= partition->data.name;
-    if (!fs::copy_file(fromPath, toPath, ec))
+    // copy to the reserved memory area
+    if (flags == O_RDONLY)
     {
-        MSG_ERR("RWRequest: Failed to copy file from '%s' to '%s': %d\n",
-                fromPath.c_str(), toPath.c_str(), ec.value());
-        elog<InternalFailure>();
+        len = std::min(partition.data.actual - offset, len);
+        memcpy(buf, (char *)map + offset, len);
     }
-
-    rc = Request::open(toPath.c_str(), O_RDWR);
-
-    if (rc != ReturnCode::SUCCESS)
+    else
     {
-        MSG_ERR("RWRequest: Failed to open file at '%s': %d\n", toPath.c_str(),
-                static_cast<uint8_t>(rc));
-        elog<InternalFailure>();
+        // if the asked offset + no of bytes to read is greater
+        // then size of the partition file then throw error.
+        //
+        // FIXME: Don't use .actual, use  (.size << ctx->block_size_shift),
+        // otherwise we can't grow the size of the data to fill the partition
+        if ((base + offset + len) > (base + partition.data.actual))
+        {
+            munmap(map, partition.data.actual);
+            close(fd);
+
+            /* FIXME: offset calculation */
+            std::stringstream err;
+            err << "Request size 0x" << std::hex << len << " from offset 0x"
+                << std::hex << offset << " exceeds the partition size 0x"
+                << std::hex << partition.data.actual;
+            throw OutOfBoundsOffset(err.str());
+        }
+        memcpy((char *)map + offset, buf, len);
+        set_flash_bytemap(ctx, base + offset, len, FLASH_DIRTY);
     }
+    munmap(map, partition.data.actual);
+    close(fd);
 
-    return partition;
+    return len;
 }
 
 } // namespace virtual_pnor
diff --git a/pnor_partition.hpp b/pnor_partition.hpp
index a11a921..87bae3b 100644
--- a/pnor_partition.hpp
+++ b/pnor_partition.hpp
@@ -2,7 +2,12 @@
 /* Copyright (C) 2018 IBM Corp. */
 #pragma once
 
+extern "C" {
+#include "mbox.h"
+};
+
 #include "mboxd_pnor_partition_table.h"
+#include "pnor_partition_table.hpp"
 #include <fcntl.h>
 #include <string>
 #include <unistd.h>
@@ -68,51 +73,46 @@ enum class ReturnCode : uint8_t
 class Request
 {
   public:
-    Request() = default;
+    /** @brief Construct a flash access request
+     *
+     *  @param[in] ctx - The mbox context used to process the request
+     *  @param[in] offset - The absolute offset into the flash device as
+     *                      provided by the mbox message associated with the
+     *                      request
+     *
+     *  The class does not take ownership of the ctx pointer. The lifetime of
+     *  the ctx pointer must strictly exceed the lifetime of the class
+     *  instance.
+     */
+    Request(struct mbox_context* ctx, size_t offset) :
+        ctx(ctx), partition(ctx->vpnor->table->partition(offset)),
+        base(partition.data.base << ctx->block_size_shift),
+        offset(offset - base)
+    {
+    }
     Request(const Request&) = delete;
     Request& operator=(const Request&) = delete;
     Request(Request&&) = default;
     Request& operator=(Request&&) = default;
     ~Request() = default;
 
-    openpower::file::Descriptor fd;
+    ssize_t read(void* dst, size_t len)
+    {
+        return fulfil(dst, len, O_RDONLY);
+    }
 
-  protected:
-    /** @brief opens the partition file
-     *
-     *  @param[in] filePath - Absolute file path.
-     *  @param[in] mode - File open mode.
-     */
-    ReturnCode open(const std::string& filePath, int mode);
+    ssize_t write(void* dst, size_t len)
+    {
+        return fulfil(dst, len, O_RDWR);
+    }
 
-    /** @brief returns the partition file path associated with the offset.
+  private:
+    /** @brief Returns the partition file path associated with the offset.
      *
-     *  @param[in] context - The mbox context pointer.
-     *  @param[in] offset - The pnor offset(bytes).
-     */
-
-    std::string getPartitionFilePath(struct mbox_context* context,
-                                     uint32_t offset);
-
-    const pnor_partition* partition = nullptr;
-};
-
-/** @class RORequest
- *  @brief Represent the read request of the partition.
- *         Stores the partition meta data.
- */
-class RORequest : public Request
-{
-  public:
-    RORequest() = default;
-    RORequest(const RORequest&) = delete;
-    RORequest& operator=(const RORequest&) = delete;
-    RORequest(RORequest&&) = default;
-    RORequest& operator=(RORequest&&) = default;
-    ~RORequest(){};
-
-    /** @brief opens the partition file associated with the offset
-     *         in read only mode and gets the partition details.
+     *  The search strategy for the partition file depends on the value of the
+     *  flags parameter.
+     *
+     *  For the O_RDONLY case:
      *
      *  1.  Depending on the partition type,tries to open the file
      *      from the associated partition(RW/PRSV/RO).
@@ -122,29 +122,7 @@ class RORequest : public Request
      *  1b. if the file not found in the read only partition then
      *      throw exception.
      *
-     *  @param[in] context - The mbox context pointer.
-     *  @param[in] offset - The pnor offset(bytes).
-     */
-    const pnor_partition* getPartitionInfo(struct mbox_context* context,
-                                           uint32_t offset);
-};
-
-/** @class RWRequest
- *  @brief Represent the write request of the partition.
- *         Stores the partition meta data.
- */
-class RWRequest : public Request
-{
-  public:
-    RWRequest() = default;
-    RWRequest(const RWRequest&) = delete;
-    RWRequest& operator=(const RWRequest&) = delete;
-    RWRequest(RWRequest&&) = default;
-    RWRequest& operator=(RWRequest&&) = default;
-    ~RWRequest(){};
-
-    /** @brief opens the partition file associated with the offset
-     *         in write mode and gets the parttition details.
+     * For the O_RDWR case:
      *
      *  1.  Depending on the partition type tries to open the file
      *      from the associated partition.
@@ -152,13 +130,29 @@ class RWRequest : public Request
      *      then copy the file from the read only partition to the (RW/PRSV)
      *      partition depending on the partition type.
      *  1b. if the file not found in the read only partition then throw
-     * exception.
+     *      exception.
+     *
+     *  @param[in] flags - The flags that will be used to open the file. Must
+     *                     be one of O_RDONLY or O_RDWR.
+     *
+     *  Post-condition: The file described by the returned path exists
+     *
+     *  Throws: std::filesystem_error, std::bad_alloc
+     */
+    std::experimental::filesystem::path getPartitionFilePath(int flags);
+
+    /** @brief Fill dst with the content of the partition relative to offset.
      *
-     *  @param[in] context - The mbox context pointer.
      *  @param[in] offset - The pnor offset(bytes).
+     *  @param[out] dst - The buffer to fill with partition data
+     *  @param[in] len - The length of the destination buffer
      */
-    const pnor_partition* getPartitionInfo(struct mbox_context* context,
-                                           uint32_t offset);
+    ssize_t fulfil(void* dst, size_t len, int flags);
+
+    struct mbox_context* ctx;
+    const pnor_partition& partition;
+    size_t base;
+    size_t offset;
 };
 
 } // namespace virtual_pnor