Skip to content

Refactoring of the pbkdf2 crate.#20

Closed
jfrimmel wants to merge 11 commits intoRustCrypto:masterfrom
jfrimmel:cleanup
Closed

Refactoring of the pbkdf2 crate.#20
jfrimmel wants to merge 11 commits intoRustCrypto:masterfrom
jfrimmel:cleanup

Conversation

@jfrimmel
Copy link
Copy Markdown

This PR applies refactoring to the pbkdf2 crate, especially for the simple module.
It is best viewed commit by commit.

@jfrimmel
Copy link
Copy Markdown
Author

Do you want to keep the minimum Rust version of 1.22? Some dependencies (e.g. rand) could also be updated, if the MSRV is bumped

Copy link
Copy Markdown
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good!

But I think we can add a small optimization.


// check the format of the input: there may be no tokens before the first
// and after the last `$`, tokens must have correct information and length.
let (count, salt, hash) = match parts[..] {
Copy link
Copy Markdown
Member

@newpavlov newpavlov Aug 18, 2019

Choose a reason for hiding this comment

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

I think you can remove the allocation and be compatible with Rust 1.22 by using:

let mut p = hashed_value.split('$');
let buf = [
    p.next(), p.next(), p.next(), p.next(),
    p.next(), p.next(), p.next(), p.next(),
];
let (count, salt, hash) = match buf {
    [
        Some(""), Some("rpbkdf2"), Some("0"), Some(count),
        Some(salt), Some(hash), Some(""), None,
    ] => (count, salt, hash),
    _ => return Err(CheckError::InvalidFormat),
};

Also it looks like Err(CheckError::InvalidFormat)? is discouraged (e.g. by clippy), so we probably better to replace it with an explicit return.

Copy link
Copy Markdown
Author

@jfrimmel jfrimmel Aug 18, 2019

Choose a reason for hiding this comment

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

Thank you for your suggestion. I will remove the unnecessary allocation (I wasn't happy with it, too) by applying your changes 😄
Unfortunately, I don't think, that Rust 1.22 will support this code as well.

Edit: but I have an Idea how to do this in Rust 1.22. I'll update this PR soon

@fubuloubu
Copy link
Copy Markdown

This will fix #17, correct?

@jfrimmel
Copy link
Copy Markdown
Author

This will fix #17, correct?

Unfortunately no, since this PR does not increase the MSRV. This would be required to use Rand 0.7.

@tarcieri
Copy link
Copy Markdown
Member

@jfrimmel can you rebase? we've bumped MSRV to 1.41+

@newpavlov
Copy link
Copy Markdown
Member

I've included your refactor into #33.

@newpavlov newpavlov closed this Jun 10, 2020
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.

4 participants