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

[Maxim Levitsky]

On Thu, 2019-08-22 at 16:32 +0200, Max Reitz wrote:
> On 22.08.19 01:59, Maxim Levitsky wrote:
> > On Tue, 2019-08-20 at 19:36 +0200, Max Reitz wrote:
> > > On 14.08.19 22:22, Maxim Levitsky wrote:
> > > > This is also a preparation for key read/write/erase functions
> > > > 
> > > > * use master key len from the header
> > > > * prefer to use crypto params in the QCryptoBlockLUKS
> > > >   over passing them as function arguments
> > > > * define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME
> > > > * Add comments to various crypto parameters in the QCryptoBlockLUKS
> > > > 
> > > > Signed-off-by: Maxim Levitsky <address@hidden>
> > > > ---
> > > >  crypto/block-luks.c | 213 ++++++++++++++++++++++----------------------
> > > >  1 file changed, 105 insertions(+), 108 deletions(-)
> 
> 
> [...]
> 
> > > > @@ -410,21 +430,15 @@ 
> > > > qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher,
> > > >   */
> > > >  static int
> > > >  qcrypto_block_luks_load_key(QCryptoBlock *block,
> > > > -                            QCryptoBlockLUKSKeySlot *slot,
> > > > +                            uint slot_idx,
> > > 
> > > Did you use uint on purpose or do you mean a plain “unsigned”?
> > 
> > Well there are just 8 slots, but yea I don't mind to make this an unsigned 
> > int.
> 
> My point was that “uint” is not a standard C type.

There are lot of non standard types, like the u8/u8/u32/u64 I used to use in 
the kernel,
so I kind of missed that. Won't use that type anymore :-)


> 
> [...]
> 
> > > > @@ -930,6 +922,17 @@ qcrypto_block_luks_create(QCryptoBlock *block,
> > > >              luks_opts.has_ivgen_hash_alg = true;
> > > >          }
> > > >      }
> > > > +
> > > > +    luks = g_new0(QCryptoBlockLUKS, 1);
> > > > +    block->opaque = luks;
> > > > +
> > > > +    luks->cipher_alg = luks_opts.cipher_alg;
> > > > +    luks->cipher_mode = luks_opts.cipher_mode;
> > > > +    luks->ivgen_alg = luks_opts.ivgen_alg;
> > > > +    luks->ivgen_hash_alg = luks_opts.ivgen_hash_alg;
> > > > +    luks->hash_alg = luks_opts.hash_alg;
> > > > +
> > > > +
> > > 
> > > Why did you pull this up?  Now @luks is leaked in both of the next error
> > > paths.
> > 
> > This is because the purpose of these fields changed. As Daniel explained to 
> > me
> > they were initially added after the fact to serve as a cache of into to 
> > present in qemu-img info callback.
> > But now I use these everywhere in the code so I won't them to be correct as 
> > soon as possible to minimize
> > the risk of calling some function that uses these and would read garbage.
> 
> I get that, but I was wondering why you pulled the allocation of @luks
> up above the next two conditional blocks.  Allocating and initializing
> there should have worked just fine.
Yea, I didn't have to, just thought that putting the initialization as above
as possible is a good thing for future.


> 
> Max
> 

Best regards,
        Maxim Levitsky