[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-block] [PATCH 02/13] qcrypto-luks: misc refactori
From: |
Maxim Levitsky |
Subject: |
Re: [Qemu-devel] [Qemu-block] [PATCH 02/13] qcrypto-luks: misc refactoring |
Date: |
Thu, 22 Aug 2019 14:04:28 +0300 |
On Thu, 2019-08-22 at 11:29 +0100, Daniel P. Berrangé wrote:
> 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
>
The point of adding the masterkeylen was that IMHO it wasn't clear that
luks->header.key_bytes is the master key length.
However defining something like
qcrypto_block_luks_master_key_len(QCryptoBlockLUKS *luks)
is way longer, and will force me to wrap just too many places in the code to
keep 80
character limit.
Now I am thinking of other ways to make this thing better:
1. How about adding luks->masterkeylen and using it. luks state already has
several parsed values from the header, so using another one wouldn't hurt?
2. how about renaming the luks->header.key_bytes to
luks->header->master_key_len?
Best regards,
Maxim Levitsky
- Re: [Qemu-devel] [PATCH 01/13] block-crypto: misc refactoring, (continued)
Re: [Qemu-devel] [PATCH 01/13] block-crypto: misc refactoring, Daniel P . Berrangé, 2019/08/21
[Qemu-devel] [PATCH 02/13] qcrypto-luks: misc refactoring, Maxim Levitsky, 2019/08/14
Re: [Qemu-devel] [PATCH 02/13] qcrypto-luks: misc refactoring, Max Reitz, 2019/08/20
[Qemu-devel] [PATCH 04/13] qcrypto-luks: refactoring: simplify the math used for keyslot locations, Maxim Levitsky, 2019/08/14
[Qemu-devel] [PATCH 05/13] qcrypto-luks: clear the masterkey and password before freeing them always, Maxim Levitsky, 2019/08/14