process: add IPMI error return mechanism and update errors
The IPMI packet validation code must return specific IPMI errors
corresponding to what error has occurred instead of the invalid command
error.
Update all IPMI handler pieces to return more specific errors.
Change-Id: I8d21e92015d84cc0880e3b83991aed7288e19eab
Signed-off-by: Patrick Venture <venture@google.com>
diff --git a/ipmi.cpp b/ipmi.cpp
index fb5f120..e2e0fef 100644
--- a/ipmi.cpp
+++ b/ipmi.cpp
@@ -105,7 +105,7 @@
std::string blobId = mgr->getBlobId(request.blobIdx);
if (blobId == "")
{
- return IPMI_CC_INVALID;
+ return IPMI_CC_INVALID_FIELD_REQUEST;
}
/* TODO(venture): Need to do a hard-code check against the maximum
@@ -130,13 +130,13 @@
request->blobId, (requestLen - sizeof(struct BmcBlobOpenTx)));
if (path.empty())
{
- return IPMI_CC_INVALID;
+ return IPMI_CC_REQ_DATA_LEN_INVALID;
}
/* Attempt to open. */
if (!mgr->open(request->flags, path, &session))
{
- return IPMI_CC_INVALID;
+ return IPMI_CC_UNSPECIFIED_ERROR;
}
struct BmcBlobOpenRx reply;
@@ -158,7 +158,7 @@
/* Attempt to close. */
if (!mgr->close(request.sessionId))
{
- return IPMI_CC_INVALID;
+ return IPMI_CC_UNSPECIFIED_ERROR;
}
(*dataLen) = 0;
@@ -175,13 +175,13 @@
request->blobId, (requestLen - sizeof(struct BmcBlobDeleteTx)));
if (path.empty())
{
- return IPMI_CC_INVALID;
+ return IPMI_CC_REQ_DATA_LEN_INVALID;
}
/* Attempt to delete. */
if (!mgr->deleteBlob(path))
{
- return IPMI_CC_INVALID;
+ return IPMI_CC_UNSPECIFIED_ERROR;
}
(*dataLen) = 0;
@@ -220,14 +220,14 @@
request->blobId, (requestLen - sizeof(struct BmcBlobStatTx)));
if (path.empty())
{
- return IPMI_CC_INVALID;
+ return IPMI_CC_REQ_DATA_LEN_INVALID;
}
/* Attempt to stat. */
struct BlobMeta meta;
if (!mgr->stat(path, &meta))
{
- return IPMI_CC_INVALID;
+ return IPMI_CC_UNSPECIFIED_ERROR;
}
return returnStatBlob(&meta, replyCmdBuf, dataLen);
@@ -244,7 +244,7 @@
if (!mgr->stat(request.sessionId, &meta))
{
- return IPMI_CC_INVALID;
+ return IPMI_CC_UNSPECIFIED_ERROR;
}
return returnStatBlob(&meta, replyCmdBuf, dataLen);
@@ -259,7 +259,7 @@
/* Sanity check the commitDataLen */
if (request->commitDataLen > (requestLen - sizeof(struct BmcBlobCommitTx)))
{
- return IPMI_CC_INVALID;
+ return IPMI_CC_REQ_DATA_LEN_INVALID;
}
std::vector<uint8_t> data(request->commitDataLen);
@@ -267,7 +267,7 @@
if (!mgr->commit(request->sessionId, data))
{
- return IPMI_CC_INVALID;
+ return IPMI_CC_UNSPECIFIED_ERROR;
}
(*dataLen) = 0;
@@ -317,7 +317,7 @@
/* Attempt to write the bytes. */
if (!mgr->write(request->sessionId, request->offset, data))
{
- return IPMI_CC_INVALID;
+ return IPMI_CC_UNSPECIFIED_ERROR;
}
return IPMI_CC_OK;
@@ -342,7 +342,7 @@
/* Attempt to write the bytes. */
if (!mgr->writeMeta(request.sessionId, request.offset, data))
{
- return IPMI_CC_INVALID;
+ return IPMI_CC_UNSPECIFIED_ERROR;
}
return IPMI_CC_OK;
diff --git a/main.cpp b/main.cpp
index dda9dec..4328567 100644
--- a/main.cpp
+++ b/main.cpp
@@ -47,15 +47,17 @@
*/
if ((*dataLen) < 1)
{
- return IPMI_CC_INVALID;
+ return IPMI_CC_REQ_DATA_LEN_INVALID;
}
+ /* on failure rc is set to the corresponding IPMI error. */
+ ipmi_ret_t rc = IPMI_CC_OK;
Crc16 crc;
IpmiBlobHandler command =
- validateBlobCommand(&crc, reqBuf, replyCmdBuf, dataLen);
+ validateBlobCommand(&crc, reqBuf, replyCmdBuf, dataLen, &rc);
if (command == nullptr)
{
- return IPMI_CC_INVALID;
+ return rc;
}
return processBlobCommand(command, getBlobManager(), &crc, reqBuf,
diff --git a/process.cpp b/process.cpp
index 245da69..7e6410d 100644
--- a/process.cpp
+++ b/process.cpp
@@ -48,7 +48,8 @@
};
IpmiBlobHandler validateBlobCommand(CrcInterface* crc, const uint8_t* reqBuf,
- uint8_t* replyCmdBuf, size_t* dataLen)
+ uint8_t* replyCmdBuf, size_t* dataLen,
+ ipmi_ret_t* code)
{
size_t requestLength = (*dataLen);
/* We know dataLen is at least 1 already */
@@ -57,6 +58,7 @@
/* Validate it's at least well-formed. */
if (!validateRequestLength(command, requestLength))
{
+ *code = IPMI_CC_REQ_DATA_LEN_INVALID;
return nullptr;
}
@@ -66,6 +68,7 @@
/* Verify the request includes: command, crc16, data */
if (requestLength < sizeof(struct BmcRx))
{
+ *code = IPMI_CC_REQ_DATA_LEN_INVALID;
return nullptr;
}
@@ -91,6 +94,7 @@
/* Crc expected but didn't match. */
if (crcValue != crc->get())
{
+ *code = IPMI_CC_UNSPECIFIED_ERROR;
return nullptr;
}
}
@@ -99,6 +103,7 @@
auto found = handlers.find(command);
if (found == handlers.end())
{
+ *code = IPMI_CC_INVALID_FIELD_REQUEST;
return nullptr;
}
@@ -126,7 +131,7 @@
/* The response, if it has one byte, has three, to include the crc16. */
if (replyLength < (sizeof(uint16_t) + 1))
{
- return IPMI_CC_INVALID;
+ return IPMI_CC_UNSPECIFIED_ERROR;
}
/* The command, whatever it was, replied, so let's set the CRC. */
diff --git a/process.hpp b/process.hpp
index 8f874dc..0e377bc 100644
--- a/process.hpp
+++ b/process.hpp
@@ -22,10 +22,12 @@
* @param[in,out] replyCmdBuf - a pointer to the ipmi reply packet buffer.
* @param[in,out] dataLen - initially the request length, set to reply length
* on return.
- * @return the ipmi command handler.
+ * @param[out] code - set to the IPMI error on failure, otherwise unset.
+ * @return the ipmi command handler, or nullptr on failure.
*/
IpmiBlobHandler validateBlobCommand(CrcInterface* crc, const uint8_t* reqBuf,
- uint8_t* replyCmdBuf, size_t* dataLen);
+ uint8_t* replyCmdBuf, size_t* dataLen,
+ ipmi_ret_t* code);
/**
* Call the IPMI command and process the result, including running the CRC
diff --git a/test/ipmi_close_unittest.cpp b/test/ipmi_close_unittest.cpp
index f315ea0..edfddca 100644
--- a/test/ipmi_close_unittest.cpp
+++ b/test/ipmi_close_unittest.cpp
@@ -38,7 +38,8 @@
std::memcpy(request, &req, sizeof(req));
EXPECT_CALL(mgr, close(sessionId)).WillOnce(Return(false));
- EXPECT_EQ(IPMI_CC_INVALID, closeBlob(&mgr, request, reply, &dataLen));
+ EXPECT_EQ(IPMI_CC_UNSPECIFIED_ERROR,
+ closeBlob(&mgr, request, reply, &dataLen));
}
TEST(BlobCloseTest, BlobClosedReturnsSuccess)
diff --git a/test/ipmi_commit_unittest.cpp b/test/ipmi_commit_unittest.cpp
index 34ff10d..30f78bf 100644
--- a/test/ipmi_commit_unittest.cpp
+++ b/test/ipmi_commit_unittest.cpp
@@ -35,7 +35,8 @@
dataLen = sizeof(struct BmcBlobCommitTx);
- EXPECT_EQ(IPMI_CC_INVALID, commitBlob(&mgr, request, reply, &dataLen));
+ EXPECT_EQ(IPMI_CC_REQ_DATA_LEN_INVALID,
+ commitBlob(&mgr, request, reply, &dataLen));
}
TEST(BlobCommitTest, ValidCommitNoDataHandlerRejectsReturnsFailure)
@@ -57,7 +58,8 @@
EXPECT_CALL(mgr, commit(req->sessionId, _)).WillOnce(Return(false));
- EXPECT_EQ(IPMI_CC_INVALID, commitBlob(&mgr, request, reply, &dataLen));
+ EXPECT_EQ(IPMI_CC_UNSPECIFIED_ERROR,
+ commitBlob(&mgr, request, reply, &dataLen));
}
TEST(BlobCommitTest, ValidCommitNoDataHandlerAcceptsReturnsSuccess)
diff --git a/test/ipmi_delete_unittest.cpp b/test/ipmi_delete_unittest.cpp
index 409635d..6985eb1 100644
--- a/test/ipmi_delete_unittest.cpp
+++ b/test/ipmi_delete_unittest.cpp
@@ -36,7 +36,8 @@
dataLen = sizeof(struct BmcBlobDeleteTx) + blobId.length();
- EXPECT_EQ(IPMI_CC_INVALID, deleteBlob(&mgr, request, reply, &dataLen));
+ EXPECT_EQ(IPMI_CC_REQ_DATA_LEN_INVALID,
+ deleteBlob(&mgr, request, reply, &dataLen));
}
TEST(BlobDeleteTest, RequestRejectedReturnsFailure)
@@ -60,7 +61,8 @@
EXPECT_CALL(mgr, deleteBlob(StrEq(blobId))).WillOnce(Return(false));
- EXPECT_EQ(IPMI_CC_INVALID, deleteBlob(&mgr, request, reply, &dataLen));
+ EXPECT_EQ(IPMI_CC_UNSPECIFIED_ERROR,
+ deleteBlob(&mgr, request, reply, &dataLen));
}
TEST(BlobDeleteTest, BlobDeleteReturnsOk)
diff --git a/test/ipmi_enumerate_unittest.cpp b/test/ipmi_enumerate_unittest.cpp
index 041a8cb..184c686 100644
--- a/test/ipmi_enumerate_unittest.cpp
+++ b/test/ipmi_enumerate_unittest.cpp
@@ -31,7 +31,8 @@
EXPECT_CALL(mgr, getBlobId(req.blobIdx)).WillOnce(Return(""));
- EXPECT_EQ(IPMI_CC_INVALID, enumerateBlob(&mgr, request, reply, &dataLen));
+ EXPECT_EQ(IPMI_CC_INVALID_FIELD_REQUEST,
+ enumerateBlob(&mgr, request, reply, &dataLen));
}
TEST(BlobEnumerateTest, BoringRequestByIdAndReceive)
diff --git a/test/ipmi_open_unittest.cpp b/test/ipmi_open_unittest.cpp
index 523025d..704c144 100644
--- a/test/ipmi_open_unittest.cpp
+++ b/test/ipmi_open_unittest.cpp
@@ -39,7 +39,8 @@
dataLen = sizeof(struct BmcBlobOpenTx) + blobId.length();
- EXPECT_EQ(IPMI_CC_INVALID, openBlob(&mgr, request, reply, &dataLen));
+ EXPECT_EQ(IPMI_CC_REQ_DATA_LEN_INVALID,
+ openBlob(&mgr, request, reply, &dataLen));
}
TEST(BlobOpenTest, RequestRejectedReturnsFailure)
@@ -65,7 +66,8 @@
EXPECT_CALL(mgr, open(req->flags, StrEq(blobId), _))
.WillOnce(Return(false));
- EXPECT_EQ(IPMI_CC_INVALID, openBlob(&mgr, request, reply, &dataLen));
+ EXPECT_EQ(IPMI_CC_UNSPECIFIED_ERROR,
+ openBlob(&mgr, request, reply, &dataLen));
}
TEST(BlobOpenTest, BlobOpenReturnsOk)
diff --git a/test/ipmi_sessionstat_unittest.cpp b/test/ipmi_sessionstat_unittest.cpp
index 70c36ec..a71252c 100644
--- a/test/ipmi_sessionstat_unittest.cpp
+++ b/test/ipmi_sessionstat_unittest.cpp
@@ -37,7 +37,8 @@
Matcher<struct BlobMeta*>(_)))
.WillOnce(Return(false));
- EXPECT_EQ(IPMI_CC_INVALID, sessionStatBlob(&mgr, request, reply, &dataLen));
+ EXPECT_EQ(IPMI_CC_UNSPECIFIED_ERROR,
+ sessionStatBlob(&mgr, request, reply, &dataLen));
}
TEST(BlobSessionStatTest, RequestSucceedsNoMetadata)
diff --git a/test/ipmi_stat_unittest.cpp b/test/ipmi_stat_unittest.cpp
index 75f90c4..e7ff277 100644
--- a/test/ipmi_stat_unittest.cpp
+++ b/test/ipmi_stat_unittest.cpp
@@ -39,7 +39,8 @@
dataLen = sizeof(struct BmcBlobStatTx) + blobId.length();
- EXPECT_EQ(IPMI_CC_INVALID, statBlob(&mgr, request, reply, &dataLen));
+ EXPECT_EQ(IPMI_CC_REQ_DATA_LEN_INVALID,
+ statBlob(&mgr, request, reply, &dataLen));
}
TEST(BlobStatTest, RequestRejectedReturnsFailure)
@@ -65,7 +66,8 @@
Matcher<struct BlobMeta*>(_)))
.WillOnce(Return(false));
- EXPECT_EQ(IPMI_CC_INVALID, statBlob(&mgr, request, reply, &dataLen));
+ EXPECT_EQ(IPMI_CC_UNSPECIFIED_ERROR,
+ statBlob(&mgr, request, reply, &dataLen));
}
TEST(BlobStatTest, RequestSucceedsNoMetadata)
diff --git a/test/ipmi_write_unittest.cpp b/test/ipmi_write_unittest.cpp
index 7b5e45f..99615af 100644
--- a/test/ipmi_write_unittest.cpp
+++ b/test/ipmi_write_unittest.cpp
@@ -40,7 +40,8 @@
ElementsAreArray(expectedBytes, sizeof(expectedBytes))))
.WillOnce(Return(false));
- EXPECT_EQ(IPMI_CC_INVALID, writeBlob(&mgr, request, reply, &dataLen));
+ EXPECT_EQ(IPMI_CC_UNSPECIFIED_ERROR,
+ writeBlob(&mgr, request, reply, &dataLen));
}
TEST(BlobWriteTest, ManagerReturnsTrueWriteSucceeds)
diff --git a/test/ipmi_writemeta_unittest.cpp b/test/ipmi_writemeta_unittest.cpp
index 2dfbe59..2d63452 100644
--- a/test/ipmi_writemeta_unittest.cpp
+++ b/test/ipmi_writemeta_unittest.cpp
@@ -39,7 +39,8 @@
ElementsAreArray(expectedBytes, sizeof(expectedBytes))))
.WillOnce(Return(false));
- EXPECT_EQ(IPMI_CC_INVALID, writeMeta(&mgr, request, reply, &dataLen));
+ EXPECT_EQ(IPMI_CC_UNSPECIFIED_ERROR,
+ writeMeta(&mgr, request, reply, &dataLen));
}
TEST(BlobWriteMetaTest, ManagerReturnsTrueWriteSucceeds)
diff --git a/test/process_unittest.cpp b/test/process_unittest.cpp
index 63e6f5e..4f8c9a6 100644
--- a/test/process_unittest.cpp
+++ b/test/process_unittest.cpp
@@ -57,8 +57,11 @@
request[0] = 0xff; // There is no command 0xff.
dataLen = sizeof(uint8_t); // There is no payload for CRC.
+ ipmi_ret_t rc;
- EXPECT_EQ(nullptr, validateBlobCommand(&crc, request, reply, &dataLen));
+ EXPECT_EQ(nullptr,
+ validateBlobCommand(&crc, request, reply, &dataLen, &rc));
+ EXPECT_EQ(IPMI_CC_INVALID_FIELD_REQUEST, rc);
}
TEST(ValidateBlobCommandTest, ValidCommandWithoutPayload)
@@ -72,8 +75,10 @@
request[0] = BlobOEMCommands::bmcBlobGetCount;
dataLen = sizeof(uint8_t); // There is no payload for CRC.
+ ipmi_ret_t rc;
- IpmiBlobHandler res = validateBlobCommand(&crc, request, reply, &dataLen);
+ IpmiBlobHandler res =
+ validateBlobCommand(&crc, request, reply, &dataLen, &rc);
EXPECT_FALSE(res == nullptr);
EqualFunctions(getBlobCount, res);
}
@@ -91,8 +96,11 @@
request[0] = BlobOEMCommands::bmcBlobGetCount;
dataLen = sizeof(uint8_t) + sizeof(uint16_t);
// There is a payload, but there are insufficient bytes.
+ ipmi_ret_t rc;
- EXPECT_EQ(nullptr, validateBlobCommand(&crc, request, reply, &dataLen));
+ EXPECT_EQ(nullptr,
+ validateBlobCommand(&crc, request, reply, &dataLen, &rc));
+ EXPECT_EQ(IPMI_CC_REQ_DATA_LEN_INVALID, rc);
}
TEST(ValidateBlobCommandTest, WithPayloadAndInvalidCrc)
@@ -125,7 +133,11 @@
}));
EXPECT_CALL(crc, get()).WillOnce(Return(0x1234));
- EXPECT_EQ(nullptr, validateBlobCommand(&crc, request, reply, &dataLen));
+ ipmi_ret_t rc;
+
+ EXPECT_EQ(nullptr,
+ validateBlobCommand(&crc, request, reply, &dataLen, &rc));
+ EXPECT_EQ(IPMI_CC_UNSPECIFIED_ERROR, rc);
}
TEST(ValidateBlobCommandTest, WithPayloadAndValidCrc)
@@ -158,7 +170,10 @@
}));
EXPECT_CALL(crc, get()).WillOnce(Return(0x3412));
- IpmiBlobHandler res = validateBlobCommand(&crc, request, reply, &dataLen);
+ ipmi_ret_t rc;
+
+ IpmiBlobHandler res =
+ validateBlobCommand(&crc, request, reply, &dataLen, &rc);
EXPECT_FALSE(res == nullptr);
EqualFunctions(writeBlob, res);
}
@@ -181,8 +196,10 @@
uint8_t reply[MAX_IPMI_BUFFER] = {0};
dataLen = sizeof(request);
+ ipmi_ret_t rc;
- IpmiBlobHandler res = validateBlobCommand(&crc, request, reply, &dataLen);
+ IpmiBlobHandler res =
+ validateBlobCommand(&crc, request, reply, &dataLen, &rc);
EXPECT_FALSE(res == nullptr);
EqualFunctions(openBlob, res);
}
@@ -250,7 +267,7 @@
dataLen = sizeof(request);
- EXPECT_EQ(IPMI_CC_INVALID,
+ EXPECT_EQ(IPMI_CC_UNSPECIFIED_ERROR,
processBlobCommand(h, &manager, &crc, request, reply, &dataLen));
}