Re: [Qemu-devel] [Qemu-block] [PATCH 02/13] qcrypto-luks: misc refactoring

[Maxim Levitsky]

On Thu, 2019-08-22 at 12:10 +0100, Daniel P. Berrangé wrote:
> On Thu, Aug 22, 2019 at 02:04:28PM +0300, Maxim Levitsky wrote:
> > 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?
> 
> With those the parsed values are actually a different format from the
> header values, so it makes sense to have duplication.  Duplication
> just for sake of having a different name will just be confusing
> with some code using one field & some code using the other field
> when they are identical.
> 
> > 2. how about renaming the luks->header.key_bytes to 
> > luks->header->master_key_len?
> 
> This is fine.

Roger that!
Best regards,
        Maxim Levitsky


> 
> Regards,
> Daniel