Enable cppcoreguidelines-avoid-goto checks
We've been pretty good about this one, the only usages seem to be in
nvme sensor and added recently. To move to more RAII compliant
containers, this commit creates a FileHandle class, which accepts either
a file path and a mode, or a direct file handle, to be handled via RAII.
Ideally we wouldn't have to create this ourselves, and could rely on
some library, but the alternatives all have compromises that are worse.
boost::asio::random_access_file requires iouring to be enabled, and
isn't available until boost 0.78, which we haven't rebased to yet.
https://www.boost.org/doc/libs/develop/doc/html/boost_asio/reference/random_access_file.html
Once available, this is what I would hope that a bunch of the nvme stuff
evolves into to be much simpler.
boost::iostream::file_descriptor requires linking against
boost::iostream, which would bloat our binary size for a lot of things
we don't use. There is a file_descriptor.cpp that we could compile
directly to solve this, but the existing yocto boost project (nor any
other distro project) installs these files for use, which would
complicate this solution
std::fstream doesn't allow direct access to the file handle, which would
be required for streaming operations.
https://www.boost.org/doc/libs/1_35_0/libs/iostreams/doc/installation.html
https://www.boost.org/doc/libs/1_78_0/libs/iostreams/doc/classes/file_descriptor.html#file_descriptor
A very similar class exists within ipmid here:
https://github.com/openbmc/phosphor-host-ipmid/blob/master/user_channel/file.hpp
This class differs in a couple minor ways, in that it throws exceptions
on open failures (similar to the boost classes), accepts the input by
std::filesystem::path, instead of string, and supports std::move, which
is required for the pipe usage.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I8be2419783e41e8f529030bb3d05c2e7aa890307
diff --git a/.clang-tidy b/.clang-tidy
index c8fe523..052456d 100644
--- a/.clang-tidy
+++ b/.clang-tidy
@@ -186,6 +186,7 @@
clang-analyzer-webkit.NoUncountedMemberChecker,
clang-analyzer-webkit.RefCntblBaseVirtualDtor,
cppcoreguidelines-avoid-c-arrays,
+cppcoreguidelines-avoid-goto,
cppcoreguidelines-init-variables,
cppcoreguidelines-pro-type-vararg,
misc-misplaced-const,
diff --git a/include/FileHandle.hpp b/include/FileHandle.hpp
new file mode 100644
index 0000000..46e5539
--- /dev/null
+++ b/include/FileHandle.hpp
@@ -0,0 +1,27 @@
+#pragma once
+
+#include <filesystem>
+#include <ios>
+
+// An RAII compliant object for holding a posix file descriptor
+class FileHandle
+{
+ private:
+ int fd;
+
+ public:
+ FileHandle() = delete;
+ FileHandle(const FileHandle&) = delete;
+ FileHandle& operator=(const FileHandle&) = delete;
+ FileHandle(FileHandle&&) noexcept;
+ FileHandle& operator=(FileHandle&&) noexcept;
+
+ explicit FileHandle(const std::filesystem::path& name,
+ std::ios_base::openmode mode = std::ios_base::in |
+ std::ios_base::out);
+
+ explicit FileHandle(int fd);
+
+ ~FileHandle();
+ int handle();
+};
\ No newline at end of file
diff --git a/meson.build b/meson.build
index c3ab050..8003366 100644
--- a/meson.build
+++ b/meson.build
@@ -88,7 +88,11 @@
utils_a = static_library(
'utils_a',
- ['src/Utils.cpp', 'src/SensorPaths.cpp'],
+ [
+ 'src/FileHandle.cpp',
+ 'src/SensorPaths.cpp',
+ 'src/Utils.cpp',
+ ],
dependencies: default_deps,
implicit_include_directories: false,
include_directories: 'include',
diff --git a/src/FileHandle.cpp b/src/FileHandle.cpp
new file mode 100644
index 0000000..6724698
--- /dev/null
+++ b/src/FileHandle.cpp
@@ -0,0 +1,50 @@
+#include <fcntl.h>
+#include <unistd.h>
+
+#include <FileHandle.hpp>
+
+#include <iostream>
+#include <stdexcept>
+
+FileHandle::FileHandle(const std::filesystem::path& name,
+ std::ios_base::openmode mode) :
+ // NOLINTNEXTLINE(cppcoreguidelines-pro-type-vararg)
+ fd(open(name.c_str(), mode))
+{
+ if (fd < 0)
+ {
+ throw std::out_of_range(name.string() + " failed to open");
+ }
+}
+
+FileHandle::FileHandle(int fdIn) : fd(fdIn){};
+
+FileHandle::FileHandle(FileHandle&& in) noexcept
+{
+ fd = in.fd;
+ in.fd = -1;
+}
+
+FileHandle& FileHandle::operator=(FileHandle&& in) noexcept
+{
+ fd = in.fd;
+ in.fd = -1;
+ return *this;
+}
+
+FileHandle::~FileHandle()
+{
+ if (fd)
+ {
+ int r = close(fd);
+ if (r < 0)
+ {
+ std::cerr << "Failed to close fd " << std::to_string(fd);
+ }
+ }
+}
+
+int FileHandle::handle()
+{
+ return fd;
+}
diff --git a/src/NVMeBasicContext.cpp b/src/NVMeBasicContext.cpp
index 0e04b16..a99287f 100644
--- a/src/NVMeBasicContext.cpp
+++ b/src/NVMeBasicContext.cpp
@@ -4,6 +4,7 @@
#include <sys/ioctl.h>
#include <unistd.h>
+#include <FileHandle.hpp>
#include <boost/asio/read.hpp>
#include <boost/asio/streambuf.hpp>
#include <boost/asio/write.hpp>
@@ -61,65 +62,44 @@
static ssize_t execBasicQuery(int bus, uint8_t addr, uint8_t cmd,
std::vector<uint8_t>& resp)
{
- int rc = 0;
int32_t size = 0;
- std::string devpath = "/dev/i2c-" + std::to_string(bus);
-
- // NOLINTNEXTLINE(cppcoreguidelines-pro-type-vararg)
- int dev = ::open(devpath.c_str(), O_RDWR);
- if (dev == -1)
- {
- std::cerr << "Failed to open bus device " << devpath << ": "
- << strerror(errno) << "\n";
- return -errno;
- }
+ std::filesystem::path devpath = "/dev/i2c-" + std::to_string(bus);
+ FileHandle fileHandle(devpath);
/* Select the target device */
// NOLINTNEXTLINE(cppcoreguidelines-pro-type-vararg)
- if (::ioctl(dev, I2C_SLAVE, addr) == -1)
+ if (::ioctl(fileHandle.handle(), I2C_SLAVE, addr) == -1)
{
- rc = -errno;
std::cerr << "Failed to configure device address 0x" << std::hex
<< (int)addr << " for bus " << std::dec << bus << ": "
<< strerror(errno) << "\n";
- goto cleanup_fds;
+
+ return -errno;
}
resp.reserve(UINT8_MAX + 1);
/* Issue the NVMe MI basic command */
- size = i2c_smbus_read_block_data(dev, cmd, resp.data());
+ size = i2c_smbus_read_block_data(fileHandle.handle(), cmd, resp.data());
if (size < 0)
{
- rc = size;
std::cerr << "Failed to read block data from device 0x" << std::hex
<< (int)addr << " on bus " << std::dec << bus << ": "
<< strerror(errno) << "\n";
- goto cleanup_fds;
+ return size;
}
- else if (size > UINT8_MAX + 1)
+ if (size > UINT8_MAX + 1)
{
- rc = -EBADMSG;
std::cerr << "Unexpected message length from device 0x" << std::hex
<< (int)addr << " on bus " << std::dec << bus << ": " << size
<< " (" << UINT8_MAX << ")\n";
- goto cleanup_fds;
+ return -EBADMSG;
}
- rc = size;
-
-cleanup_fds:
- if (::close(dev) == -1)
- {
- std::cerr << "Failed to close device descriptor " << std::dec << dev
- << " for bus " << std::dec << bus << ": " << strerror(errno)
- << "\n";
- }
-
- return rc;
+ return size;
}
-static ssize_t processBasicQueryStream(int in, int out)
+static ssize_t processBasicQueryStream(FileHandle& in, FileHandle& out)
{
std::vector<uint8_t> resp{};
ssize_t rc = 0;
@@ -135,17 +115,16 @@
std::array<uint8_t, sizeof(uint32_t) + 1 + 1> req{};
/* Read the command parameters */
- if ((rc = ::read(in, req.data(), req.size())) !=
- static_cast<ssize_t>(req.size()))
+ ssize_t rc = ::read(in.handle(), req.data(), req.size());
+ if (rc != static_cast<ssize_t>(req.size()))
{
- assert(rc < 1);
- rc = rc ? -errno : -EIO;
- if (errno)
+ std::cerr << "Failed to read request from in descriptor "
+ << strerror(errno) << "\n";
+ if (rc)
{
- std::cerr << "Failed to read request from in descriptor ("
- << std::dec << in << "): " << strerror(errno) << "\n";
+ return -errno;
}
- goto done;
+ return -EIO;
}
decodeBasicQuery(req, bus, device, offset);
@@ -171,47 +150,41 @@
}
/* Write out the response length */
- if ((rc = ::write(out, &len, sizeof(len))) != sizeof(len))
+ rc = ::write(out.handle(), &len, sizeof(len));
+ if (rc != sizeof(len))
{
- assert(rc < 1);
- rc = rc ? -errno : -EIO;
std::cerr << "Failed to write block (" << std::dec << len
- << ") length to out descriptor (" << std::dec << out
- << "): " << strerror(-rc) << "\n";
- goto done;
+ << ") length to out descriptor: "
+ << strerror(static_cast<int>(-rc)) << "\n";
+ if (rc)
+ {
+ return -errno;
+ }
+ return -EIO;
}
/* Write out the response data */
- uint8_t* cursor = resp.data();
- while (len > 0)
+ std::vector<uint8_t>::iterator cursor = resp.begin();
+ while (cursor != resp.end())
{
- ssize_t egress = ::write(out, cursor, len);
+ size_t lenRemaining = std::distance(cursor, resp.end());
+ ssize_t egress = ::write(out.handle(), &(*cursor), lenRemaining);
if (egress == -1)
{
- rc = -errno;
std::cerr << "Failed to write block data of length " << std::dec
- << len << " to out pipe: " << strerror(errno) << "\n";
- goto done;
+ << lenRemaining << " to out pipe: " << strerror(errno)
+ << "\n";
+ if (rc)
+ {
+ return -errno;
+ }
+ return -EIO;
}
cursor += egress;
- len -= egress;
}
}
-done:
- if (::close(in) == -1)
- {
- std::cerr << "Failed to close in descriptor " << std::dec << in << ": "
- << strerror(errno) << "\n";
- }
-
- if (::close(out) == -1)
- {
- std::cerr << "Failed to close out descriptor " << std::dec << in << ": "
- << strerror(errno) << "\n";
- }
-
return rc;
}
@@ -252,11 +225,12 @@
}
reqStream.assign(requestPipe[1]);
- int streamIn = requestPipe[0];
- int streamOut = responsePipe[1];
+ FileHandle streamIn(requestPipe[0]);
+ FileHandle streamOut(responsePipe[1]);
respStream.assign(responsePipe[0]);
- std::thread thread([streamIn, streamOut]() {
+ std::thread thread([streamIn{std::move(streamIn)},
+ streamOut{std::move(streamOut)}]() mutable {
ssize_t rc = 0;
if ((rc = processBasicQueryStream(streamIn, streamOut)) < 0)
@@ -265,18 +239,6 @@
<< strerror(static_cast<int>(-rc)) << "\n";
}
- if (::close(streamIn) == -1)
- {
- std::cerr << "Failed to close streamIn descriptor: "
- << strerror(errno) << "\n";
- }
-
- if (::close(streamOut) == -1)
- {
- std::cerr << "Failed to close streamOut descriptor: "
- << strerror(errno) << "\n";
- }
-
std::cerr << "Terminating basic query thread\n";
});
thread.detach();