Unexpected conversion for _ in filename()
#60
Comments
|
|
|
Yeah, we can't apply sd_bus_path_encode() / decode() to all paths as it changes the meaning of all |
|
Yep, this is a problem; We should probably discuss the solution on discord, because I'm on the fence with what to do about it. The issue is that underscore has special meaning in dbus paths when they're encoded as the precursor to an escape sequence. Unfortunately, we also seem to use underscores in the example above where they're not escaped. In theory bios_active should've been escaped as bios_5Factive, which would've caused this to decode properly. Instead, it's being put on dbus as-is, which is decoding to the hex code for 0xAC, which is extended ascii for a code that we don't support. A quick look through the services on my desktop shows that most services sidestep this by creating camel cased path identifiers for well known dbus paths, although things like org.freedesktop.systemd1, which need to handle freeform strings definitely do escape underscores. The options I see going forward are:
Option 2 would be my preference. PS, can you point out the code that's actually broken here? Ie, what API is using filename() and expecting to get bios_active back out? I didn't know there was widespread usage of filename() yet, and the only usage of bios_active that I'm able to find is in intel-ipmi-oem, but I'm unable to find the backend implementation for it. Can you give me some pointers to what code you're looking at? |
It is the |
What is the backend code that is producing an object called "bios_active" for your system? That will need to be corrected to either publish BiosActive or bios_5Factive. Also, which bmcweb endpoint are you testing with? The /xyz apis? I don't see any specific call outs for bios_active in the redfish endpoints. |
|
The |
|
Unfortunately, we've used snake_case almost everywhere as our preferred dbus naming scheme. Even our TLDs are snake_case (org/open_power, xyz/openbmc_project). I'm sort of leaning towards we should rethink how we do escaping and we shouldn't just take in the systemd method. Looking at my own system, Notice that in network the |
|
Alright, correction. network1 does use the escape. ascii 31 = '1' and that is the name of the link. login1 uses it for session maybe, but not user? My uid is 1000, so _1000 is not escaped. But the session (At first I couldn't figure out why they decided to escape integers like that, since integers are valid and there are some odd ones like |
Could you elaborate a little bit on what you're thinking? Given the number of allowed characters in dbus paths, there aren't that many options that are even possible.
If I remember right, you can't start a dbus path component with an integer (or something like that, the details are fuzzy). |
FWIW, object paths segments can start with integers. Bus Names and Members have this prohibition but not objects. https://dbus.freedesktop.org/doc/dbus-specification.html#message-protocol-marshaling-object-path |
|
For another data point, this issue is breaking a lot of stuff in intel platforms due to the chassis board name containing an underscore which is not escaped by EntityManager. The board name is decoded by bmcweb (into a broken looking name) when generating the ChassisCollection links, but not encoded again when following that link to get the Chassis resource, so the Redfish Chassis is inaccessible. I don't really follow why we should want to encode all path names. Underscores are perfectly OK in D-Bus paths, and we happen to be using it in a lot of places (since |
This sounds like a bug. I can look into it. bmcweb should be encoding and decoding at every step.
Because there are things we want to represent on dbus that aren't just a-z0-9_. In this specific case, there's a desire to have dashes in sensor names.
There's a patchset working on exactly what you want. The initial half-done stab at this is here: |
|
Ed, The invocation of the sdbusplus::message::object_path is the point at which I found the D-Bus Chassis path getting mangled. The object has AC_Baseboard on the way in, and "path" ends up with AC\ufffdseboard coming out. |
|
Yep, you're right, filename() is "mangling" the path by applying escaping rules. James invented his own escaping rules in entity-manager when he found this issue initially (all non-encodable characters become underscores) which doesn't really cover the full range, and causes entity-manager to crash in weird ways if you mess up the json even a little. That's largely what I'm trying to fix with the encoding and decoding. https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/42628 should makes the Redfish directory structure traversal again, although the names will still be weird, but considering nobody should be using IDs directly, that's less of a concern (something I'm still wanting to fix with the sdbusplus patch though. I need to figure out how to test it (none of my systems put underscores in chassis names). |
|
I'd like to understand a bit better... |
|
In my first submission to this thread I was pointing out that the name was mangled by sdbusplus::message::object_path path(object); before filename() was even called. It was the ::object_path constructor doing the damage to filename() in my investigation of the issue. |
|
I would like to see us move to to systemd naming conventions, which rely on the underscore character and hex codes to escape paths. I cannot imagine this is everyone's direction, and I'm sure there will be some differing of opinions, but it's something we should definitely discuss. We've already had issues with daemons that have to interact with systemd services having to do some kind of manual escaping by hand (also getting it wrong). Also, things like entity-manager invented their own escaping mechanisms (mostly because we didn't know any sort of standard existed at the time). This is causing issues where certain configs can crash entity-manager because of a bad name in a config file, which is unfortunate because EM is supposed to have a user facing dbus interface. |
A common anti pattern is using endswith or contains. This got compounded recently because of openbmc/sdbusplus#60 where filename now contains decoding logic. This at least should fix the issue to make the Redfish tree walkable again. Tested: Loaded up a WFP_Baseboard on qemu, and verified that /redfish/v1/Chassis/WFP_Baseboard traversed correctly. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I55cc389a773e4b594668a4bca466efc33cc9cb7c
The filename() is expected to return the DBus object path's "filename", however, it converts underscore
_unexpectedly.E.g. the object "/xyz/openbmc_project/software/bios_active"'s
filename()shall bebios_active, butsdbusplus::message::object_path::filename()converts it tobios\xACtive.This issue is found in g220a's HW CI after rebasing the code to upstream 06dc92d6f.
The text was updated successfully, but these errors were encountered: