Re: [PATCH v3 00/14] LUKS: encryption slot management using amend interface

[Daniel P . Berrangé]

On Sun, May 03, 2020 at 09:43:10PM +0300, Maxim Levitsky wrote:
> Hi!
> Here is the updated series of my patches, incorporating all the feedback I 
> received.
> 
> This implements the API interface that we agreed upon except that I merged the
> LUKSKeyslotActive/LUKSKeyslotInactive union into a struct because otherwise
> I need nested unions which are not supported currently by QAPI parser.
> This didn't change the API and thus once support for nested unions is there,
> it can always be implemented in backward compatible way.
> 
> I hope that this series will finally be considered for merging, since I am 
> somewhat running
> out of time to finish this task.
> 
> Patches are strictly divided by topic to 3 groups, and each group depends on 
> former groups.
> 
> * Patches 1,2 implement qcrypto generic amend interface, including definition
>   of structs used in crypto.json and implement this in luks crypto driver
>   Nothing is exposed to the user at this stage
> 
> * Patches 3-9 use the code from patches 1,2 to implement qemu-img amend based 
> encryption slot management
>   for luks and for qcow2, and add a bunch of iotests to cover that.
> 
> * Patches 10-13 add x-blockdev-amend (I'll drop the -x prefix if you like), 
> and wire it
>   to luks and qcow2 driver to implement qmp based encryption slot management 
> also using
>   the code from patches 1,2, and also add a bunch of iotests to cover this.
> 
> Tested with -raw,-qcow2 and -luks iotests and 'make check'
> 
> V3: rebased, addressed most of the review feedback.
> For now I kept the slot bitmap code since I am not sure that replacing it 
> will be better.

I'm still of the opinion that the bitmaps code should be replaced.
With this current design we are iterating over the slot of keys slots
4 times, doing different checks/logic in each iteration. This is really
not nice for understanding how the code works. IMHO we should be iterating
at most twice - once to validate the requested configuration, and once
to apply the requested configuration.   Even if there si duplication of
logic in between the delete/add key codepaths, I think it will be better
as the logic for each operation will be in one place.


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 :|