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

Add support for /dev/devices #311

Merged
merged 3 commits into from
Aug 25, 2024
Merged

Conversation

yamaura
Copy link
Contributor

@yamaura yamaura commented Jun 1, 2024

Thank you for this amazing library! I found it incredibly useful. I implemented parser of /proc/devices and test.

Notes:

  • The type for major has been same with the Linux kernel code.
  • While BlockDeviceEntry can be disabled via CONFIG_BLOCK, but it has not been made an Option.

@eminence
Copy link
Owner

Thank you! I'm sorry for taking so long to notice and review this PR.

This looks good, only a few comments:

While BlockDeviceEntry can be disabled via CONFIG_BLOCK, but it has not been made an Option.

The API you've added here looks good, but can you update the doc comment to say that the block_devices list can be empty if the kernel doesn't support block devices?

Also, these structs have #[allow(non_snake_case)] but that doesn't seem to be needed.

@yamaura
Copy link
Contributor Author

yamaura commented Aug 7, 2024

I've fixed the items you pointed out. Please take a look!

@eminence
Copy link
Owner

Thanks! Quick question: why is CharDeviceEntry::major a u32 while BlockDeviceEntry::major an i32 ?

@yamaura
Copy link
Contributor Author

yamaura commented Aug 23, 2024

The difference in types is aligned with Linux kernel code. See below links.
I think it would be fine to use the same type, considering convenience for user of this library.

If you have a policy on whether to prioritize user convenience or align with the kernel regarding types, please let me know.

https://github.com/torvalds/linux/blob/master/fs/char_dev.c#L36
https://github.com/torvalds/linux/blob/master/block/genhd.c#L160

@eminence
Copy link
Owner

Thanks for those links. My general policy is to prioritize alignment with the kernel (though I think I deviate from that in a few spots). A comment in the code with these links would be useful to anyone reading this code in the future, but that can happen later.

Thanks for the pull request!

@eminence eminence merged commit 6c959b6 into eminence:master Aug 25, 2024
6 checks passed
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