CommandProcessor: add fd argument to ProcessInput()
Various commands (such as commands which request a log
dump, or which write a blob to the log) will need to
provide a file descriptor argument. Accordingly, we
revise ProcessInput(), to support such an argument.
Add tests that verify that a provided FD argument
is closed when ProcessInput() returns. As noted in
the test implementation, these tests aren't truly
unit tests. But they're the best we can reasonably
do, given how unique_fd works.
Along the way: rename ProcessInput() to
ProcessCommand(). The new name is more consistent
with various bits of documentation (comments and
type names).
Bug: 32093334
Test: ./runtests.sh
Change-Id: Ib5845dc4ec13d10564894c4dbe7b486e3351ed22
diff --git a/command_processor.cpp b/command_processor.cpp
index 96eb5f4..db2cc5c 100644
--- a/command_processor.cpp
+++ b/command_processor.cpp
@@ -18,6 +18,7 @@
#include <utility>
#include "android-base/logging.h"
+#include "android-base/unique_fd.h"
#include "wifilogd/byte_buffer.h"
#include "wifilogd/command_processor.h"
@@ -27,6 +28,8 @@
namespace android {
namespace wifilogd {
+using ::android::base::unique_fd;
+
using local_utils::CopyFromBufferOrDie;
using local_utils::GetMaxVal;
@@ -37,8 +40,10 @@
std::unique_ptr<Os> os)
: current_log_buffer_(buffer_size_bytes), os_(std::move(os)) {}
-bool CommandProcessor::ProcessInput(const void* input_buffer,
- size_t n_bytes_read) {
+bool CommandProcessor::ProcessCommand(const void* input_buffer,
+ size_t n_bytes_read, int fd) {
+ unique_fd wrapped_fd(fd);
+
if (n_bytes_read < sizeof(protocol::Command)) {
return false;
}
diff --git a/command_processor.h b/command_processor.h
index 9e4e7e5..0c2ae66 100644
--- a/command_processor.h
+++ b/command_processor.h
@@ -38,9 +38,18 @@
// This method allows tests to provide a MockOs.
CommandProcessor(size_t buffer_size_bytes, std::unique_ptr<Os> os);
- // Processes the given command. The effect of this call depends on the
- // contents of |input_buf|.
- bool ProcessInput(NONNULL const void* input_buf, size_t n_bytes_read);
+ // Processes the given command, with the given file descriptor. The effect of
+ // this call depends on the contents of |input_buf|. In particular, depending
+ // on the command, |fd| may be used for reading or writing, or |fd| may be
+ // ignored. However, |fd| is guaranteed to be closed before ProcessCommand()
+ // returns, in all cases.
+ //
+ // (Ideally, we might want to take |fd| as a unique_fd. Unfortunately,
+ // GoogleMock doesn't deal well with move-only parameters. And we'll
+ // want to mock this method, eventually.
+ // https://github.com/google/googletest/issues/395)
+ bool ProcessCommand(NONNULL const void* input_buf, size_t n_bytes_read,
+ int fd);
private:
struct TimestampHeader {
diff --git a/os.h b/os.h
index 4776d3a..ebadd84 100644
--- a/os.h
+++ b/os.h
@@ -46,6 +46,8 @@
uint32_t nsecs;
};
+ static constexpr int kInvalidFd = -1;
+
// Constructs an Os instance.
Os();
diff --git a/tests/command_processor_unittest.cpp b/tests/command_processor_unittest.cpp
index 9413464..3dc0579 100644
--- a/tests/command_processor_unittest.cpp
+++ b/tests/command_processor_unittest.cpp
@@ -14,10 +14,13 @@
* limitations under the License.
*/
+#include <unistd.h>
+
#include <memory>
#include <string>
#include <utility>
+#include "android-base/unique_fd.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -32,6 +35,9 @@
namespace wifilogd {
namespace {
+using ::android::base::unique_fd;
+using ::testing::_;
+using ::testing::AnyNumber;
using ::testing::StrictMock;
using local_utils::GetMaxVal;
@@ -104,8 +110,8 @@
EXPECT_CALL(*os_, GetTimestamp(CLOCK_MONOTONIC));
EXPECT_CALL(*os_, GetTimestamp(CLOCK_BOOTTIME));
EXPECT_CALL(*os_, GetTimestamp(CLOCK_REALTIME));
- return command_processor_->ProcessInput(command_buffer.data(),
- command_buffer.size());
+ return command_processor_->ProcessCommand(
+ command_buffer.data(), command_buffer.size(), Os::kInvalidFd);
}
bool SendAsciiMessage(const std::string& tag, const std::string& message) {
@@ -121,61 +127,61 @@
} // namespace
// A valid ASCII message should, of course, be processed successfully.
-TEST_F(CommandProcessorTest, ProcessInputOnValidAsciiMessageSucceeds) {
+TEST_F(CommandProcessorTest, ProcessCommandOnValidAsciiMessageSucceeds) {
EXPECT_TRUE(SendAsciiMessage("tag", "message"));
}
-// If the buffer given to ProcessInput() is shorter than a protocol::Command,
+// If the buffer given to ProcessCommand() is shorter than a protocol::Command,
// then we discard the data.
TEST_F(CommandProcessorTest,
- ProcessInputOnAsciiMessageShorterThanCommandFails) {
+ ProcessCommandOnAsciiMessageShorterThanCommandFails) {
const CommandBuffer& command_buffer(
BuildAsciiMessageCommand("tag", "message"));
- EXPECT_FALSE(command_processor_->ProcessInput(command_buffer.data(),
- sizeof(protocol::Command) - 1));
+ EXPECT_FALSE(command_processor_->ProcessCommand(
+ command_buffer.data(), sizeof(protocol::Command) - 1, Os::kInvalidFd));
}
// In all other cases, we save the data we got, and will try to salvage the
// contents when dumping.
-TEST_F(CommandProcessorTest, ProcessInputOnAsciiMessageWithEmtpyTagSucceeds) {
+TEST_F(CommandProcessorTest, ProcessCommandOnAsciiMessageWithEmtpyTagSucceeds) {
EXPECT_TRUE(SendAsciiMessage("", "message"));
}
TEST_F(CommandProcessorTest,
- ProcessInputOnAsciiMessageWithEmptyMessageSucceeds) {
+ ProcessCommandOnAsciiMessageWithEmptyMessageSucceeds) {
EXPECT_TRUE(SendAsciiMessage("tag", ""));
}
TEST_F(CommandProcessorTest,
- ProcessInputOnAsciiMessageWithEmptyTagAndMessageSucceeds) {
+ ProcessCommandOnAsciiMessageWithEmptyTagAndMessageSucceeds) {
EXPECT_TRUE(SendAsciiMessage("", ""));
}
TEST_F(CommandProcessorTest,
- ProcessInputOnAsciiMessageWithBadCommandLengthSucceeds) {
+ ProcessCommandOnAsciiMessageWithBadCommandLengthSucceeds) {
EXPECT_TRUE(SendAsciiMessageWithSizeAdjustments("tag", "message", 1, 0, 0));
EXPECT_TRUE(SendAsciiMessageWithSizeAdjustments("tag", "message", -1, 0, 0));
}
TEST_F(CommandProcessorTest,
- ProcessInputOnAsciiMessageWithBadTagLengthSucceeds) {
+ ProcessCommandOnAsciiMessageWithBadTagLengthSucceeds) {
EXPECT_TRUE(SendAsciiMessageWithSizeAdjustments("tag", "message", 0, 1, 0));
EXPECT_TRUE(SendAsciiMessageWithSizeAdjustments("tag", "message", 0, -1, 0));
}
TEST_F(CommandProcessorTest,
- ProcessInputOnAsciiMessageWithBadMessageLengthSucceeds) {
+ ProcessCommandOnAsciiMessageWithBadMessageLengthSucceeds) {
EXPECT_TRUE(SendAsciiMessageWithSizeAdjustments("tag", "message", 0, 0, 1));
EXPECT_TRUE(SendAsciiMessageWithSizeAdjustments("tag", "message", 0, 0, -1));
}
-TEST_F(CommandProcessorTest, ProcessInputOnOverlyLargeAsciiMessageSucceeds) {
+TEST_F(CommandProcessorTest, ProcessCommandOnOverlyLargeAsciiMessageSucceeds) {
const std::string tag{"tag"};
EXPECT_TRUE(SendAsciiMessage(
tag, std::string(kMaxAsciiMessagePayloadLen - tag.size() + 1, '.')));
}
-TEST_F(CommandProcessorTest, ProcessInputSucceedsEvenAfterFillingBuffer) {
+TEST_F(CommandProcessorTest, ProcessCommandSucceedsEvenAfterFillingBuffer) {
const std::string tag{"tag"};
const std::string message(kMaxAsciiMessagePayloadLen - tag.size(), '.');
for (size_t cumulative_payload_bytes = 0;
@@ -185,5 +191,37 @@
}
}
+// Strictly speaking, this is not a unit test. But there's no easy way to get
+// unique_fd to call on an instance of our Os.
+TEST_F(CommandProcessorTest, ProcessCommandClosesFd) {
+ int pipe_fds[2];
+ ASSERT_EQ(0, pipe(pipe_fds));
+
+ const unique_fd our_fd{pipe_fds[0]};
+ const int their_fd = pipe_fds[1];
+ const CommandBuffer& command_buffer(
+ BuildAsciiMessageCommand("tag", "message"));
+ EXPECT_CALL(*os_, GetTimestamp(_)).Times(AnyNumber());
+ EXPECT_TRUE(command_processor_->ProcessCommand(
+ command_buffer.data(), command_buffer.size(), their_fd));
+ EXPECT_EQ(-1, close(their_fd));
+ EXPECT_EQ(EBADF, errno);
+}
+
+// Strictly speaking, this is not a unit test. But there's no easy way to get
+// unique_fd to call on an instance of our Os.
+TEST_F(CommandProcessorTest, ProcessCommandClosesFdEvenOnFailure) {
+ int pipe_fds[2];
+ ASSERT_EQ(0, pipe(pipe_fds));
+
+ const unique_fd our_fd{pipe_fds[0]};
+ const int their_fd = pipe_fds[1];
+ const CommandBuffer command_buffer;
+ EXPECT_FALSE(command_processor_->ProcessCommand(
+ command_buffer.data(), command_buffer.size(), their_fd));
+ EXPECT_EQ(-1, close(their_fd));
+ EXPECT_EQ(EBADF, errno);
+}
+
} // namespace wifilogd
} // namespace android
