On Fri, 2019-11-08 at 14:12 +0100, Max Reitz wrote: > On 08.11.19 12:04, Maxim Levitsky wrote: > > On Fri, 2019-11-08 at 11:49 +0100, Max Reitz wrote: > > > On 08.11.19 10:30, Maxim Levitsky wrote: > > > > On Fri, 2019-10-04 at 20:41 +0200, Max Reitz wrote: > > > > > On 13.09.19 00:30, Maxim Levitsky wrote: > > > > > > This implements the encryption key management > > > > > > using the generic code in qcrypto layer > > > > > > (currently only for qemu-img amend) > > > > > > > > > > > > This code adds another 'write_func' because the initialization > > > > > > write_func works directly on the underlying file, > > > > > > because during the creation, there is no open instance > > > > > > of the luks driver, but during regular use, we have it, > > > > > > and should use it instead. > > > > > > > > > > > > > > > > > > This commit also adds a 'hack/workaround' I and Kevin Wolf > > > > > > (thanks) > > > > > > made to make the driver still support write sharing, > > > > > > but be safe against concurrent metadata update (the keys) > > > > > > Eventually write sharing for luks driver will be deprecated > > > > > > and removed together with this hack. > > > > > > > > > > > > The hack is that we ask (as a format driver) for > > > > > > BLK_PERM_CONSISTENT_READ always > > > > > > (technically always unless opened with BDRV_O_NO_IO) > > > > > > > > > > > > and then when we want to update the keys, we > > > > > > unshare that permission. So if someone else > > > > > > has the image open, even readonly, this will fail. > > > > > > > > > > > > Also thanks to Daniel Berrange for the variant of > > > > > > that hack that involves asking for read, > > > > > > rather that write permission > > > > > > > > > > > > Signed-off-by: Maxim Levitsky <mlevi...@redhat.com> > > > > > > --- > > > > > > block/crypto.c | 118 > > > > > > +++++++++++++++++++++++++++++++++++++++++++++++-- > > > > > > 1 file changed, 115 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/block/crypto.c b/block/crypto.c > > > > > > index a6a3e1f1d8..f42fa057e6 100644 > > > > > > --- a/block/crypto.c > > > > > > +++ b/block/crypto.c > > > > > > @@ -36,6 +36,7 @@ typedef struct BlockCrypto BlockCrypto; > > > > > > > > > > > > struct BlockCrypto { > > > > > > QCryptoBlock *block; > > > > > > + bool updating_keys; > > > > > > }; > > > > > > > > > > > > > > > > > > @@ -70,6 +71,24 @@ static ssize_t > > > > > > block_crypto_read_func(QCryptoBlock *block, > > > > > > return ret; > > > > > > } > > > > > > > > > > > > +static ssize_t block_crypto_write_func(QCryptoBlock *block, > > > > > > + size_t offset, > > > > > > + const uint8_t *buf, > > > > > > + size_t buflen, > > > > > > + void *opaque, > > > > > > + Error **errp) > > > > > > > > > > There’s already a function of this name for creation. > > > > > > > > There is a long story why two write functions are needed. > > > > i tried to use only one, but at the end I and Daniel both agreed > > > > that its just better to have two functions. > > > > > > > > The reason is that during creation, the luks BlockDriverState doesn't > > > > exist yet, > > > > and so the creation routine basically just writes to the underlying > > > > protocol driver. > > > > > > > > Thats is why the block_crypto_create_write_func receives a BlockBackend > > > > pointer, > > > > to which the BlockDriverState of the underlying protocol driver is > > > > inserted. > > > > > > > > > > > > On the other hand, for amend, the luks block device is open, and it > > > > only knows > > > > about its own BlockDriverState, and thus the io should be done on > > > > bs->file > > > > > > > > So instead of trying to coerce a single callback to do both of this, > > > > we decided to just have a little code duplication. > > > > > > I meant: This doesn’t compile. There’s already another function of this > > > name. > > > > > > > You probably didn't apply the 'block-crypto: misc refactoring' patch, > > or I forgot to send it. > > Maybe you forgot to mention anywhere that I should.
Now I remember that this patch was in the original re-factoring patch series, and for some reason it was dropped. Its now locally in my git tree, so I forgot that it wasn't in this patch series already. Sorry for the mess, I'll soon resend the updated patch series. Best regards, Maxim Levitsky