Skip to content
This repository has been archived by the owner on Mar 7, 2021. It is now read-only.

chrdev: Instead of passing a Range of minors, default to starting at 0 #108

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

Conversation

geofft
Copy link
Collaborator

@geofft geofft commented Jul 12, 2019

Since we're using a builder pattern we can actually implement defaults.

Since we're using a builder pattern we can actually implement defaults.
Copy link
Member

@alex alex left a comment

Choose a reason for hiding this comment

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

It'd be cool to have a test for this, probably in chrdev-region-allocation

if !name.ends_with('\x00') {
return Err(Error::EINVAL);
}

return Ok(Builder {
name,
minors,
0,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
0,
minor_start: 0,

file_ops: Vec<&'static FileOperationsVtable>,
}

impl Builder {
pub fn start_minor_at(&mut self, minor_start: u16) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should take self and return Buider, like register_device does.

@alex
Copy link
Member

alex commented Jul 15, 2019

Based on #110, I think we do need to support allocating N minors without actually registering devices on them. I think the best way to do that is a second builder method minor_count. It can be an Option<u16> on the Builder and default to len() if it's not provided.

@geofft
Copy link
Collaborator Author

geofft commented Aug 24, 2019

I don't totally understand what binderfs is doing with chrdevs. It looks it's calling alloc_chrdev_region and never calling cdev_add? and that's okay because it opening binderfs chrdevs doesn't actually go through chrdev_open because binderfs is its own filesystem? And it instead... just populates dentries or something?

I don't think that "allocate N chrdevs with operations, and M more without" is quite right, because

  • I don't think you'd want to do both (even binderfs, which has one static device named binder-control, puts together that device's inode manually).
  • File operations isn't really the right abstraction here, those devices do in fact have file operations (binder_ctl_ops, and binder_ops defined in the core binder code).

Two possible ideas for what to do with the API:

  1. Have a second builder method saying "Instead of initializing chrdevs, allocate N minors but do not initialize them." Throw a runtime error if you call both .reserve() and .register_device() on the same builder. (Or a compile-time type error, if you really want.)
  2. Split up registration and creation into two steps (which mirrors the underlying kernel API more closely, anyway) into a Registration object and a CharacterDevices object that takes ownership of the Registration. Then binderfs can just skip the second step.

But I'm not totally sure I understand what a good API looks like, here.

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

Successfully merging this pull request may close these issues.

None yet

2 participants