Skip to content
This repository has been archived by the owner on Feb 26, 2020. It is now read-only.

Changes needed for ZFS Encryption #533

Closed
wants to merge 1 commit into from
Closed

Conversation

tcaputi
Copy link
Contributor

@tcaputi tcaputi commented Feb 18, 2016

2 small changes: Deleted struct modlinkage which is currently unused and redefined in the Illumos Crypto Port (see openzfs/zfs#4329). Also added ntohll macros needed by several encryption algorithms.

@behlendorf
Copy link
Contributor

When you get a chance please rebase and squash these commits so it's clear what the actual change is for review and so they could get merged. That said, both changes look good and since they're not going to disrupt anything could be merged without much delay. (I'm all for removing dead code)

@tcaputi tcaputi force-pushed the master branch 2 times, most recently from 0e8ff4a to 8077eca Compare February 21, 2016 05:37
@tcaputi
Copy link
Contributor Author

tcaputi commented Feb 21, 2016

@behlendorf changes squashed

@tcaputi
Copy link
Contributor Author

tcaputi commented Feb 22, 2016

there is one small fix i need to make so that the htonll functions will build correctly on 32 bit systems. Will add comment when fixed.

@tcaputi
Copy link
Contributor Author

tcaputi commented Feb 22, 2016

should be ready now.

@behlendorf
Copy link
Contributor

Functionally these looks good. Although I'm curious why you opted for a static inline rather than a #define for these? Could you also explain where htonll() and ntohll() are used in the ZFS encryption keystore? I just want to make sure they're used fairly minimally.

@tcaputi
Copy link
Contributor Author

tcaputi commented Feb 22, 2016

The ICP uses these as part of the ccm and gcm implementations I can move this into the ICP if needed, but in Illumos this function was in byteorder.h. For an example of usage, see https://github.com/tcaputi/zfs/blob/master/module/icp/algs/modes/gcm.c#L45

The small fix I mentioned earlier was to change it from a #define to a static inline function. Some places in the ICP pass this function a ulonglong_t instead of a uint64_t. On 32 bit systems this causes it to complain as I saw on this build: http://build.zfsonlinux.org/builders/Ubuntu%2014.04%20i686%20%28BUILD%29/builds/3490/steps/shell_3/logs/make

I can change it to use casting if you would like. I just saw other functions (in atomic.h for instance) that used the inline functions and thought I would match that.

I will fix the style mistake in a little bit (sorry about that)

@tcaputi
Copy link
Contributor Author

tcaputi commented Feb 22, 2016

style issue fixed

@behlendorf
Copy link
Contributor

This is definitely the right spot for these, let's leave them in byteorder.h.

Using a static inline is fine with me, in fact I personally like it better because it leaves no ambiguity about the types. I just wanted to understand why.

This change LGTM, if you're happy with it as well I'll get it merged.

@tcaputi
Copy link
Contributor Author

tcaputi commented Feb 25, 2016

I just brought up an ubuntu 14.04-i386 image to be sure everything works on 32 bit. Everything seems good here so I'd say its ready for merge.

@behlendorf
Copy link
Contributor

Merged as:

18d2f56 Changes to support zfs encryption

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants