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

Fix #71890: Add support for crc32c Castagnoli's polynomial. #3913

Closed
wants to merge 1 commit into
base: master
from

Conversation

6 participants
@bramp
Copy link
Contributor

bramp commented Mar 4, 2019

This variant of crc32 is heavily used by storage systems, such as iSCSI, SCTP,
Btrfs, ext4, and is increasingly being used in APIs (such as Google Cloud
Storage, and Apache Kafka).

@petk petk added the Bugfix label Mar 4, 2019

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Mar 4, 2019

Implementation looks fine to me. Are there any official test vectors that can be included to check that the output is the right one?

@cmb69

This comment has been minimized.

Copy link
Contributor

cmb69 commented Mar 4, 2019

Should this really target PHP-7.2? After all, I don't think it's a bugfix.

@KalleZ

This comment has been minimized.

Copy link
Member

KalleZ commented Mar 4, 2019

I think this should target 7.4 if anything. From a quick glance it looks great, if we could have a few more conforming tests like @nikic mentioned, it would be great.

(Note: some bugs at the bugsweb is actually a feature request so we format it so)

@bramp

This comment has been minimized.

Copy link
Contributor Author

bramp commented Mar 4, 2019

Thanks for the quick review.

I guess I misread the instructions, as it suggested rebasing against the earliest supported version (which looked to be 7.2). Happy to rebase against master instead. However, it would be really useful (to me) if this was in 7.2. Is there anyway I can achieve that?

I looked for testing vectors, but sadly there seems to be no golden set. The original academic paper lists none, the iSCSI RFC has a few. The best I could find is Go's implementation which has ~30, but they are all testing ASCII strings.

So I can include what I've found in other implementations. Or an alternative. I write a quick program in another language (say Go), and generate samples that include all bytes (0-255) and use that to generate a canonical list.

I have a day job, so I'll make the requested changes over the next day or so. Thanks.

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Mar 4, 2019

It's probably a bit late for 7.2, but I think we can land this on 7.3 under the "small self-contained feature" provision, if @cmb69 agrees.

I think it would be good to include both those Go test vectors as well as the coverage for all bytes. If we have that shouldn't be much opportunity to go wrong :)

@cmb69

This comment has been minimized.

Copy link
Contributor

cmb69 commented Mar 4, 2019

I guess I misread the instructions, as it suggested rebasing against the earliest supported version (which looked to be 7.2). Happy to rebase against master instead. However, it would be really useful (to me) if this was in 7.2. Is there anyway I can achieve that?

7.2 would be appropriate if this was a bug; I don't think the lack of any particular hash algo is a bug, though. If you want to see this enhancement earlier than in PHP-7.4, I suggest you write to internals@lists.php.net; maybe there is some consensus about adding it to PHP-7.2 (or maybe 7.3).

@bramp bramp force-pushed the bramp:bug-71890 branch from 06e70b9 to 6cca6b3 Mar 5, 2019

@bramp bramp changed the base branch from PHP-7.2 to master Mar 5, 2019

Fix #71890: Add support for crc32c Castagnoli's polynomial.
This variant of crc32 is heavily used by storage systems, such as iSCSI, SCTP,
Btrfs, ext4, and is increasingly being used in API (such as Google Cloud
Storage, and Apache Kafka).

@bramp bramp force-pushed the bramp:bug-71890 branch from 6cca6b3 to 09757eb Mar 5, 2019

@bramp

This comment has been minimized.

Copy link
Contributor Author

bramp commented Mar 5, 2019

I've rebased to master, and I've added additional tests (using Go to generate the golden hash values). For now I'll stick with 7.4, and I'll use a crc32c implementation written in php for earlier versions.

Please take a look.

@nikic

nikic approved these changes Mar 5, 2019

Copy link
Member

nikic left a comment

Looks good to me :)

@KalleZ

KalleZ approved these changes Mar 5, 2019

@KalleZ

This comment has been minimized.

Copy link
Member

KalleZ commented Mar 5, 2019

Looks great, thank you for your work! I suggest you hit a mail to internals@ to decide on what version to target, as this is a small and self contained feature, it should be fast to decide

@smalyshev smalyshev added Enhancement and removed Bugfix labels Mar 5, 2019

@bramp

This comment has been minimized.

Copy link
Contributor Author

bramp commented Mar 10, 2019

BTW Sent email to the internals@ list, awaiting a decision on what version(s) to merge into.

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Mar 11, 2019

It seems like the sentiment on list is to not merge into release branches, so I went ahead and merged this as c79ce48 into 7.4. We can always backport if necessary (the patch would have to be slightly different for older versions anyway).

Thanks for adding this functionality :)

@nikic nikic closed this Mar 11, 2019

@bramp

This comment has been minimized.

Copy link
Contributor Author

bramp commented Mar 11, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.