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

Public ioctl api #132

Merged
merged 7 commits into from
Jun 30, 2021
Merged

Conversation

benzea
Copy link
Collaborator

@benzea benzea commented Jun 24, 2021

On top of #125 (sorry, but need fixes from there to make sense).

API is not optimal yet:

  • We could remove the main context from the IoctlBase initialisation, then pass it when attaching the device
  • Then we can hide it completely (or at least provide a good default)
  • detach requires passing the handler again; we could just hide that behind a hash table for lookup

But, works nicely.

@martinpitt
Copy link
Owner

This can be rebased now.

Instead, propagate the worker context from the testbed in the call to
register the path with the IoctlBase handler.
@benzea benzea force-pushed the benzea/public-ioctl-api branch 2 times, most recently from 54a7cb1 to e0d069e Compare June 24, 2021 16:49
@benzea
Copy link
Collaborator Author

benzea commented Jun 24, 2021

OK, did the API changes that I mentioned.

I think this is good to go now. The only downside is that it means we commit to the whole API on how data is resolved. But, considering it works well enough for the current usecases, I think that is reasonable.

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.

I gave this some more scrutiny, as it adds public API. I like this, it's very generic and flexible, great work! 👏

src/umockdev.vala Outdated Show resolved Hide resolved
src/umockdev.vala Outdated Show resolved Hide resolved
src/umockdev.vala Outdated Show resolved Hide resolved
tests/test-umockdev-vala.vala Outdated Show resolved Hide resolved
tests/test-umockdev-vala.vala Outdated Show resolved Hide resolved
src/umockdev-ioctl.vala Outdated Show resolved Hide resolved
src/umockdev-ioctl.vala Outdated Show resolved Hide resolved
src/umockdev-ioctl.vala Outdated Show resolved Hide resolved
src/umockdev-ioctl.vala Outdated Show resolved Hide resolved
src/umockdev-ioctl.vala Outdated Show resolved Hide resolved
@benzea benzea force-pushed the benzea/public-ioctl-api branch 2 times, most recently from bfbdead to 495d404 Compare June 28, 2021 13:16
@benzea
Copy link
Collaborator Author

benzea commented Jun 28, 2021

OK, I think it is quite a bit nicer now from an API point of view.

@benzea benzea force-pushed the benzea/public-ioctl-api branch from 495d404 to 520de94 Compare June 28, 2021 13:23
Benjamin Berg added 2 commits June 28, 2021 15:28
Rather than requiring explict loads and dirty marking, do this
implicitly. This means that clients only need to resolve pointers
manually and everything else is happening implicitly.

Data is only flushed back when it was modified locally. As such, this
only results in a few more round-trips as e.g. read() data will be
resolved automatically.

If desired, this can be optimized at a later point by improving the
emulation protocol.
Updating contained pointers automatically is neat, but it is not
reliable in general. Require the API user to resolve data again
explicitly as error handling would be a mess otherwise.
@benzea benzea force-pushed the benzea/public-ioctl-api branch from 520de94 to 54e5b30 Compare June 28, 2021 13:30
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.

Nice, thanks! It would be great to have a unit test that demonstrates how to handle ioctl with pointer resolving, but that can also come later -- it should already work as the SPI/evdev etc. tests use that functionality.

A few remaining leftovers only.

src/umockdev.vala Outdated Show resolved Hide resolved
src/umockdev.vala Outdated Show resolved Hide resolved
tests/test-umockdev-vala.vala Outdated Show resolved Hide resolved
tests/test-umockdev-vala.vala Outdated Show resolved Hide resolved
tests/test-umockdev-vala.vala Show resolved Hide resolved
@benzea benzea force-pushed the benzea/public-ioctl-api branch from 54e5b30 to 7b73264 Compare June 29, 2021 08:29
@benzea
Copy link
Collaborator Author

benzea commented Jun 29, 2021

Thanks!

Some explicit testing for the IoctlData stuff would be nice for reference, yes. I did add a single resolve call to the existing test.

Benjamin Berg added 4 commits June 29, 2021 18:34
Exposing this API means that users can implement full custom ioctl and
read/write emulation.

Closes: martinpitt#128
Doing this means that the vfunc is called by glib during signal
emission. It is ugly to insert the C code like this, but it works well
and it means that signal emission becomes much more well defined.
Aborting has the advantage that it will trigger a core-dump, simplifying
debugging.
@benzea benzea force-pushed the benzea/public-ioctl-api branch from 7b73264 to cf5da5d Compare June 29, 2021 16:34
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.

Yay, thank you! 👏

@martinpitt martinpitt merged commit f694ac8 into martinpitt:master Jun 30, 2021
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