Skip to content

Add support for /dev/devices#311

Merged
eminence merged 3 commits intoeminence:masterfrom
yamaura:proc_devices
Aug 25, 2024
Merged

Add support for /dev/devices#311
eminence merged 3 commits intoeminence:masterfrom
yamaura:proc_devices

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!

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