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>
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 @@
#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 @@
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 @@
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 @@
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);
+ if (!(context && context->vpnor && context->vpnor->table))
+ {
+ MSG_ERR("Trying to write data with uninitialised context!\n");
+ return -MBOX_R_SYSTEM_ERROR;
+ }
+
+ vpnor::partition::Table* table = context->vpnor->table;
+
try
{
- 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)
+ const struct pnor_partition& part = table->partition(offset);
+ if (part.data.user.data[1] & PARTITION_READONLY)
{
- MSG_ERR("Failed to map partition=%s:Error=%s\n",
- partitionInfo->data.name, strerror(errno));
-
- elog<InternalFailure>();
- }
- // copy to the mapped memory.
-
- uint32_t baseOffset = partitionInfo->data.base
- << context->block_size_shift;
-
- // 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))
- {
- 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 @@
phosphor::logging::commit<err::InternalFailure>();
return -MBOX_R_SYSTEM_ERROR;
}
- return rc;
+ return 0;
}