Fix #60 by adding encoding/decoding rules
This commit attempts to make encoding more compatible with things within
openbmc that don't encode paths per the systemd method. It does this by
forcing the first character of every path segment to be encoded, so /abc
would be encoded as /_61bc. This is then used as a mechanism to
determine if the path needs to be decoded per systemds rules.
The decode mechanisms are also modified to check whether the first
element is encoded before decoding the portion.
Looking for input on whether this is an OK path. The hope would be that
we slowly transition the system over to using the encoding mechanisms
that can handle all ascii characters, and can round trip through
encode/decode without loss. The hope is that this patch can be reversed
at some point in the future.
As an aside, I tried reverting some of the patches, but they're somewhat
ungainly to revert at this point, and would require reverting some
bmcweb patches, which, if this patch is determined to be bad, I'm happy
to do, but I'm hoping this will get us something in the interim
Tested:
unit tests added and updated to cover some of the cases. Unit tests
pass.
Change-Id: Ie9c8cd0627b8b086e8b0bb2287f28dd86eb98ee9
Signed-off-by: Ed Tanous <edtanous@google.com>
diff --git a/include/sdbusplus/message/native_types.hpp b/include/sdbusplus/message/native_types.hpp
index e13c295..06547b4 100644
--- a/include/sdbusplus/message/native_types.hpp
+++ b/include/sdbusplus/message/native_types.hpp
@@ -4,6 +4,7 @@
#include <sdbusplus/utility/memory.hpp>
+#include <array>
#include <string>
namespace sdbusplus
@@ -148,15 +149,29 @@
std::string filename() const
{
- std::string parent = parent_path();
+ size_t firstIndex = str.rfind('/');
+
+ // Dbus paths must start with /, if we don't find one, it's an error
+ if (firstIndex == std::string::npos)
+ {
+ return "";
+ }
+ firstIndex++;
+ // If we don't see that this was encoded by sdbusplus, return the naive
+ // version of the filename path.
+ const char* filename = str.c_str() + firstIndex;
+ if (*filename != '_')
+ {
+ return std::string(filename);
+ }
+
_cleanup_free_ char* out = nullptr;
- int r = sd_bus_path_decode(str.c_str(), parent.c_str(), &out);
+ int r = sd_bus_path_decode_many(filename, "%", &out);
if (r <= 0)
{
return "";
}
- std::string ret(out);
- return ret;
+ return std::string(out);
}
string_path_wrapper parent_path() const
@@ -189,6 +204,34 @@
return out;
}
out.str = encOut;
+
+ size_t firstIndex = str.size();
+ if (str != "/")
+ {
+ firstIndex++;
+ }
+
+ // Attempt to encode the first character of the path. This allows the
+ // filename() method to "detect" that this is a path that's been encoded
+ // and to decode it properly. This was needed to support a number of
+ // paths that currently dont' have any encoding, and utilize underscores
+ // Ideally this, and the equivalent code in filename() would go away
+ // when all paths are being encoded per systemds methods.
+ if (out.str[firstIndex] == '_')
+ {
+ return out;
+ }
+
+ constexpr std::array<char, 16> hex{'0', '1', '2', '3', '4', '5',
+ '6', '7', '8', '9', 'a', 'b',
+ 'c', 'd', 'e', 'f'};
+ uint8_t firstChar = static_cast<uint8_t>(*extId);
+ out.str[firstIndex] = '_';
+ std::array<char, 2> encoded{hex[(firstChar >> 4) & 0xF],
+ hex[firstChar & 0xF]};
+ out.str.insert(out.str.begin() + firstIndex + 1, encoded.begin(),
+ encoded.end());
+
return out;
}
diff --git a/test/message/types.cpp b/test/message/types.cpp
index 530fbc3..5bb6aef 100644
--- a/test/message/types.cpp
+++ b/test/message/types.cpp
@@ -46,12 +46,15 @@
ASSERT_EQ(sdbusplus::message::object_path("/abc/def").filename(), "def");
ASSERT_EQ(sdbusplus::message::object_path("/abc/").filename(), "");
ASSERT_EQ(sdbusplus::message::object_path("/abc").filename(), "abc");
+ ASSERT_EQ(sdbusplus::message::object_path("/_61bc").filename(), "abc");
ASSERT_EQ(sdbusplus::message::object_path("/").filename(), "");
ASSERT_EQ(sdbusplus::message::object_path("").filename(), "");
ASSERT_EQ(sdbusplus::message::object_path("abc").filename(), "");
ASSERT_EQ(sdbusplus::message::object_path("/_2d").filename(), "-");
ASSERT_EQ(sdbusplus::message::object_path("/_20").filename(), " ");
ASSERT_EQ(sdbusplus::message::object_path("/_2F").filename(), "/");
+ ASSERT_EQ(sdbusplus::message::object_path("/bios_active").filename(),
+ "bios_active");
}
TEST(MessageTypes, ObjectPathParent)
@@ -69,30 +72,32 @@
TEST(MessageTypes, ObjectPathOperatorSlash)
{
ASSERT_EQ(sdbusplus::message::object_path("/") / "abc",
- sdbusplus::message::object_path("/abc"));
+ sdbusplus::message::object_path("/_61bc"));
ASSERT_EQ(sdbusplus::message::object_path("/abc") / "def",
- sdbusplus::message::object_path("/abc/def"));
+ sdbusplus::message::object_path("/abc/_64ef"));
ASSERT_EQ(sdbusplus::message::object_path("/abc") / "-",
sdbusplus::message::object_path("/abc/_2d"));
ASSERT_EQ(sdbusplus::message::object_path("/abc") / " ",
sdbusplus::message::object_path("/abc/_20"));
ASSERT_EQ(sdbusplus::message::object_path("/abc") / "/",
sdbusplus::message::object_path("/abc/_2f"));
+ ASSERT_EQ(sdbusplus::message::object_path("/abc") / "ab_cd",
+ sdbusplus::message::object_path("/abc/_61b_5fcd"));
// Test the std::string overload. This is largely just for coverage
ASSERT_EQ(sdbusplus::message::object_path("/") / std::string("abc"),
- sdbusplus::message::object_path("/abc"));
+ sdbusplus::message::object_path("/_61bc"));
}
TEST(MessageTypes, ObjectPathOperatorSlashEqual)
{
sdbusplus::message::object_path path("/");
path /= "abc";
- ASSERT_EQ(path, sdbusplus::message::object_path("/abc"));
+ ASSERT_EQ(path, sdbusplus::message::object_path("/_61bc"));
sdbusplus::message::object_path path2("/");
path2 /= std::string("def");
- ASSERT_EQ(path2, sdbusplus::message::object_path("/def"));
+ ASSERT_EQ(path2, sdbusplus::message::object_path("/_64ef"));
}
TEST(MessageTypes, Signature)