On Thu, Aug 15, 2019 at 05:40:11PM -0400, John Snow wrote: > > > On 8/14/19 4:22 PM, Maxim Levitsky wrote: > > This is also a preparation for key read/write/erase functions > > > > This is a matter of taste and I am not usually reviewing LUKS patches > (So don't take me too seriously), but I would prefer not to have "misc" > patches and instead split things out by individual changes along with a > nice commit message for each change. > > > * use master key len from the header > > This touches enough lines that you could make it its own patch, I think. > > > * prefer to use crypto params in the QCryptoBlockLUKS > > over passing them as function arguments > > I think the same is true here, and highlighting which variables you are > sticking into state instead of leaving as functional parameters would be > nice to see without all the other changes. > > > * define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME > > This can likely be squashed with whichever patch of yours first needs to > use it, because it's so short. > > > * Add comments to various crypto parameters in the QCryptoBlockLUKS > > > > Can probably be squashed with item #2.
Agreed, with all these points - it is too hard to review this for correctness with everything merged in one commit, so I'll wait for v2 before reviewing much more. > > @@ -397,6 +411,12 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm > > cipher, > > } > > } > > > > +static int masterkeylen(QCryptoBlockLUKS *luks) > > +{ > > + return luks->header.key_bytes; > > +} > > + > > + > > generally QEMU uses snake_case_names; please spell as "master_key_len". Also naming convention in this file expects "qcrypto_block_luks_" prefix for all methods Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|