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

[Maxim Levitsky]

On Mon, 2020-05-04 at 11:19 +0100, Daniel P. Berrangé wrote:
> 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

All right then, I also agree kind of that these iterations aren't
that nice. I just wanted to do this in the same way the spec defines it,
which is currently kind of independent of how key add/remove operation.

I'll send updated code soon.

Best regards,
        Maxim Levitsky