Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pybridge: Stop hard-coding endian flag in DBusChannel #20122

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

pybridge: Stop hard-coding endian flag in DBusChannel #20122

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/cockpit/channels/dbus.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import errno
import json
import logging
import sys
import traceback
import xml.etree.ElementTree as ET

Expand All @@ -48,6 +49,8 @@

logger = logging.getLogger(__name__)

IS_LITTLE_ENDIAN_MACHINE = sys.byteorder == 'little'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The endianess of DBus messages is independent of the host and can differ per message. There's a flag in every dbus message header which indicates whether it's LE or BE: https://dbus.freedesktop.org/doc/dbus-specification.html#message-protocol-messages

However, it looks like sd-bus does not support messages with non-native endianess at all currently, so this is currently just broken anyway: systemd/systemd#27363

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But isn't that a strong reason to actually apply this patch? In main it always sends messages in LE, while with this patch it should send it in native endianess.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current code is definitely broken and this change makes it more likely to work on BE platforms, but it's not correct either. DBus clients which send messages in non-native endianess remain broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the documentation and I agree that this solution is not correct, even if it might work in a lot of situations. Getting the endianness from the message header would the reliable way to do it.

I did a proof of concept where I added a new API to sd-bus that could be used in systemd_ctypes to get the endianness from individual messages. I ended up becoming quite busy after doing this PR and I hope I can start working on this soon again.


# The dbusjson3 payload
#
# This channel payload type translates JSON encoded messages on a
Expand Down Expand Up @@ -174,6 +177,7 @@ class DBusChannel(Channel):
name = None
bus = None
owner = None
endianness = "<" if IS_LITTLE_ENDIAN_MACHINE else ">"

async def setup_name_owner_tracking(self):
def send_owner(owner):
Expand Down Expand Up @@ -346,10 +350,9 @@ async def do_call(self, message):
# If the method call has kicked off any signals related to
# watch processing, wait for that to be done.
async with self.watch_processing_lock:
# TODO: stop hard-coding the endian flag here.
self.send_json(
reply=[reply.get_body()], id=cookie,
flags="<" if flags is not None else None,
flags=self.endianness if flags is not None else None,
Comment on lines -352 to +355
Copy link
Member

@martinpitt martinpitt Mar 7, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some trouble understanding what flags does here. I don't see it explained in the sd_bus_message_new_method_call doc.

But still this looks upside down: I'm fairly sure that we don't want to entirely ignore flags here, i.e. on main it should have said

flags="<" if flags is None else flags,

and consequently, in this PR:

flags=self.endianess if flags is None else flags

@allisonkarlitskaya can you please double-check my logic here? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding was that the flags is a cockpit specific thing: https://github.com/cockpit-project/cockpit/blob/main/doc/protocol.md?plain=1#L468-L474

It seems to implemented in the python bridge in the similar way as it's implemented in the C bridge: https://github.com/cockpit-project/cockpit/blob/main/src/bridge/cockpitdbusjson.c#L1122-L1131

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, thanks. But this pretty much validates my claim that both the old and the new condition here is the wrong way around. So this definitively needs fixing.

type=reply.get_signature(True)) # noqa: FBT003
except BusError as error:
# actually, should send the fields from the message body
Expand Down