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

pcap: Add appropriate endianness conversions for USB pcap header #234

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

Conversation

benzea
Copy link
Collaborator

@benzea benzea commented Mar 3, 2024

The USB pcap header should always be little endian. As such, add the appropriate conversions where necessary to ensure that the data is interpreted correctly on big endian architectures.

Maybe this fixes the sparc SIGBUS problem

The USB pcap header should always be little endian. As such, add the
appropriate conversions where necessary to ensure that the data is
interpreted correctly on big endian architectures.
@benzea
Copy link
Collaborator Author

benzea commented Mar 3, 2024

Hmm, that amd64 ubuntu:devel failure does not look like a regression from this patch (failing after umockdev-testbed/libc so /umockdev-testbed/uevent/libudev). Looks a little bit odd to be honest …

@benzea
Copy link
Collaborator Author

benzea commented Mar 3, 2024

Uh, did the test previously pass on s390x? I am a bit confused that the urb is not reaped in https://download.copr.fedorainfracloud.org/results/packit/martinpitt-umockdev-234/fedora-rawhide-s390x/07099222-umockdev/builder-live.log.gz :-)

But, I think I would need to debug on an s390x machine. Just staring at the code I don't see anything wrong.

@@ -216,10 +216,10 @@ internal class IoctlUsbPcapHandler : IoctlBase {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm, there is the assert(cur_hdr.caplen). I guess that also needs a uint32.from_little_endian, but I am not entirely sure right now. It isn't going to cause issues though and we just rely on the USB header later on anyway.

@martinpitt
Copy link
Owner

that amd64 ubuntu:devel failure does not look like a regression from this patch

I haven't seen it fail recently, but I retried and it passed.

Uh, did the test previously pass on s390x?

Yes, it did. Everything has been green for a while except for nix, and that got fixed this morning. So this is definitively a regression. Note that s390x is big-endian (for that very reason I want it in PR tests).

@benzea
Copy link
Collaborator Author

benzea commented Mar 4, 2024

Yes, it did. Everything has been green for a while except for nix, and that got fixed this morning. So this is definitively a regression. Note that s390x is big-endian (for that very reason I want it in PR tests).

That is … odd. It must be pure luck that it is working, but no idea right now how that could happen (possibly because those are mostly control transfers). Anyway, later.

@benzea benzea marked this pull request as draft March 4, 2024 07:49
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.

2 participants