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.

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

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

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

Conversation

Nykseli
Copy link
Contributor

@Nykseli Nykseli commented Feb 29, 2024

Fix the issue where data is displayed incorrectly on big endian machines when using pybridge.
For example: In network manager, IPs are displayed in reverse.

martinpitt

This comment was marked as resolved.

@Nykseli

This comment was marked as resolved.

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

logger = logging.getLogger(__name__)

IS_LITTLE_ENDIAN_MACHINE = sys.byteorder == 'little'

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.

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.

Comment on lines -352 to +355
flags="<" if flags is not None else None,
flags=self.endianness if flags is not None else None,
Copy link
Member

@martinpitt martinpitt Mar 7, 2024

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.

@allisonkarlitskaya
Copy link
Member

My 2¢: the bridge ought to be taking care of this when it converts the sd_bus_message into Python types. The javascript code should never have to deal with something like endianness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants