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

Fully remove USB children when the parent USB device is removed #238

Merged
merged 3 commits into from
Aug 2, 2024
Merged

Fully remove USB children when the parent USB device is removed #238

merged 3 commits into from
Aug 2, 2024

Conversation

bobhenz-jabil
Copy link
Contributor

Suggested fix for #237

@bobhenz-jabil bobhenz-jabil mentioned this pull request Apr 24, 2024
Closed
martinpitt
martinpitt requested changes Apr 26, 2024
View reviewed changes
Copy link
Owner

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks! This also needs some unit tests, to make sure that the remove events happen, and in the expected order.

Please let me know if/when it starts to overwhelm you, then I can take over.

src/umockdev.vala Show resolved Hide resolved
src/umockdev.vala Outdated Show resolved Hide resolved
src/umockdev.vala Outdated Show resolved Hide resolved
@bobhenz-jabil
Copy link
Contributor Author

@martinpitt: Do these code changes look better?

@martinpitt
Copy link
Owner

@bobhenz-jabil sorry for the late response, it's holiday season here. The changes look good, thanks! They still need to grow some unit tests. If you want to add them, please do, otherwise I'll work on them.

But first I need to unbreak the build on Fedora, the new gcc broke vala/glib hard. I've already spent several hours on a "proper" solution, but I suppose I'll go with a quick hack for now.

@martinpitt
Copy link
Owner

I just fixed the last remaining build issue (#239), and took the liberty to rebase your branch, so that the tests can run again. Note that this is still blocked by adding unit tests for the remove event and child removal.

@bobhenz-jabil
Copy link
Contributor Author

bobhenz-jabil commented Jul 24, 2024 •
edited
Loading

@martinpitt : My apologies for the lengthy delay. I have finally gotten around to adding the following commits to this PR:

  1. Tweak an existing "uevent" test as it was manually invoking uevent() after adding a device when add_device() will invoke uevent() internally anyway.
  2. Add a test for removing a single device. (verify that uevent() was called)
  3. Add a test for removing a parent device. (verify that uevent() was called for it and each of its children)

I have also rebased on the latest main. Please let me know what you think.

martinpitt
martinpitt requested changes Jul 30, 2024
View reviewed changes
Copy link
Owner

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks! I've been meaning to add tests, but there's always something more urgent coming in between, I'm sorry!

This is now not far from landing any more, just needs some cleanups. Please also squash the unit test commits into the corresponding feature commits. I also suggest to move the "Test that add_device automatically creates an "add" uevent" commit first, as that already works on current main.

I'm happy to do that myself too if you don't have time or are fed up with this 😉

src/umockdev.vala Outdated Show resolved Hide resolved
tests/test-umockdev.py Outdated Show resolved Hide resolved
tests/test-umockdev.py Show resolved Hide resolved
@martinpitt
Copy link
Owner

@bobhenz-jabil please ignore the debian-testing failures. It's a meson regression: mesonbuild/meson#13461

@bobhenz-jabil
Test that add_device automatically creates an "add" uevent
e695709
@bobhenz-jabil
Generate a "remove" uevent
6e4a0ca 
To mirror the behavior of add_device (which generates an "add" uevent
when a device is added), now generate a "remove" uevent when a device
is removed.
@bobhenz-jabil
Formally remove children when the parent device is removed
ac750f7 
Although the directory structure for children was being removed when
the parent device was removed, `remove_device()` was not actually being
called on for the children. This meant that (for example) uevents were
not being generated for children when the parent was removed which is
different behavior than on actual hardware.
@bobhenz-jabil
Copy link
Contributor Author

@martinpitt : Fixed-up the PR per your suggestions (I believe). Let me know if I missed anything.

martinpitt
martinpitt approved these changes Aug 2, 2024
View reviewed changes
Copy link
Owner

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Excellent, many thanks! 🌟

@martinpitt martinpitt merged commit 659df15 into martinpitt:main Aug 2, 2024
21 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinpitt martinpitt martinpitt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bobhenz-jabil @martinpitt