Closed Hwatwasthat closed 2 months ago
Hi, thanks for working on this! This is a good start
I made a few in-line comments. A few additional general comments:
* Can you add a test that will actually parse a live `/proc/crypto` file? * I don't see an obvious way to construct a `CryptoTable` * There's a few very minor things that `cargo clippy` will report
For the test, you mean as in load a local file or just paste in the contents of an existing file?
I mean open /proc/crypto
and make sure it can be parsed. This is relatively common in this crate, and it's useful because I can clone this repo to any computer and just run cargo test
to make sure that everything can be parsed.
At the moment there is only a way to create one from an iterator
Sorry, I didn't quite follow this. Can you give an example about how one would create a CryptoTable
? I think probably you can impl Current for CrypotoTable
which means that you'd be able to run CryptoTable::current()
to get an instance of CryptoTable
I can implement those, I just normally try and keep it all in test and not touch the system. I can implement current for a table, currently it's only implemented that if you pass an iterator of bytes that contains a crypto table it will parse from that. I'll get a push to you this weekend that will implement these.
I think I've got everything, if you could give it a look over :) There are some clippy lints outside of the crypto sections that I have left untouched, as I don't want to mess with other folks code!
I'm looking at the /proc/crypto
file on my computer, and I see several blocks with the same name. For example:
name : stdrng
driver : drbg_nopr_hmac_sha512
module : drbg
priority : 221
refcnt : 2
selftest : passed
internal : no
type : rng
seedsize : 0
name : stdrng
driver : drbg_nopr_hmac_sha256
module : drbg
priority : 220
refcnt : 1
selftest : passed
internal : no
type : rng
seedsize : 0
Since the crypto table is a HashMap, only the last stdrng
gets saved. Is that what you intended with this code?
That was not the intention. My reckoning is that the easiest way to resolve this is to change from a Hashmap<String, CryptoBlock
to a Hashmap<String, Vec<CryptoBlock>>
. This removes the loss of duplicates but maintains the ease of the interface, having to know the exact name of a driver is probably not the most desired outcome when using this interface, and it still allows for more fine grained searching later, without the overhead of a double hashmap (I doubt there are that many duplicates).
Added in a version that has a Hashmap<String, Vec<CryptoBlock>>
. This adds some extra overhead to the type sadly but probably won't be that hurtful. Duplicates are just pushed to the vec when it happens, and that can be further broken down by the caller.
Looks pretty good on a quick skim, will do a more detailed review later. Thanks!
Hey, just checking in if you've had a chance to look and see if anything else needs an update?
Thanks for the ping, I'll take a look Very Soon Now
Thank you for the contribution, and your patience with the long review!
Hi, I have implemented parsing for the /proc/crypto file, as I noticed it wasn't implemented in the support file. I have tried to add in types where it felt relevant so it can restrict the output slightly and make handling the output a bit easier than throwing strings around. I've put in tests for various possibilities but admittedly they're far from exhaustive! I've tested on my local machine and a raspberry pi but nothing further.