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 <mlevi...@redhat.com> > > > > --- > > > > 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