On Thu, Aug 22, 2019 at 02:47:03PM +0300, Maxim Levitsky wrote:
> On Thu, 2019-08-22 at 12:16 +0100, Daniel P. Berrangé wrote:
> > On Wed, Aug 14, 2019 at 11:22:14PM +0300, Maxim Levitsky wrote:
> > > This adds qcrypto_block_manage_encryption, which
> > > is thin wrapper around manage_encryption of the crypto driver
> > > which is also added
> > >
> > > Signed-off-by: Maxim Levitsky <mlevi...@redhat.com>
> > > ---
> > > crypto/block.c | 29 +++++++++++++++++++++++++++++
> > > crypto/blockpriv.h | 9 +++++++++
> > > include/crypto/block.h | 27 +++++++++++++++++++++++++++
> > > 3 files changed, 65 insertions(+)
> > >
> > > diff --git a/crypto/block.c b/crypto/block.c
> > > index ee96759f7d..5916e49aba 100644
> > > --- a/crypto/block.c
> > > +++ b/crypto/block.c
> > > @@ -20,6 +20,7 @@
> > >
> > > #include "qemu/osdep.h"
> > > #include "qapi/error.h"
> > > +
> > > #include "blockpriv.h"
> > > #include "block-qcow.h"
> > > #include "block-luks.h"
> > > @@ -282,6 +283,34 @@ void qcrypto_block_free(QCryptoBlock *block)
> > > }
> > >
> > >
> > > +int qcrypto_block_setup_encryption(QCryptoBlock *block,
> > > + QCryptoBlockReadFunc readfunc,
> > > + QCryptoBlockWriteFunc writefunc,
> > > + void *opaque,
> > > + enum BlkSetupEncryptionAction action,
> > > + QCryptoEncryptionSetupOptions
> > > *options,
> > > + bool force,
> > > + Error **errp)
> > > +{
> > > + if (!block->driver->setup_encryption) {
> > > + error_setg(errp,
> > > + "Crypto format %s doesn't support management of
> > > encryption keys",
> > > + QCryptoBlockFormat_str(block->format));
> > > + return -1;
> > > + }
> > > +
> > > + return block->driver->setup_encryption(block,
> > > + readfunc,
> > > + writefunc,
> > > + opaque,
> > > + action,
> > > + options,
> > > + force,
> > > + errp);
> > > +}
> > > +
> > > +
> > > +
> > > typedef int (*QCryptoCipherEncDecFunc)(QCryptoCipher *cipher,
> > > const void *in,
> > > void *out,
> > > diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
> > > index 71c59cb542..804965dca3 100644
> > > --- a/crypto/blockpriv.h
> > > +++ b/crypto/blockpriv.h
> > > @@ -81,6 +81,15 @@ struct QCryptoBlockDriver {
> > >
> > > bool (*has_format)(const uint8_t *buf,
> > > size_t buflen);
> > > +
> > > + int (*setup_encryption)(QCryptoBlock *block,
> > > + QCryptoBlockReadFunc readfunc,
> > > + QCryptoBlockWriteFunc writefunc,
> > > + void *opaque,
> > > + enum BlkSetupEncryptionAction action,
> > > + QCryptoEncryptionSetupOptions *options,
> > > + bool force,
> > > + Error **errp);
> > > };
> > >
> > >
> > > diff --git a/include/crypto/block.h b/include/crypto/block.h
> > > index fe12899831..60d46e3efc 100644
> > > --- a/include/crypto/block.h
> > > +++ b/include/crypto/block.h
> > > @@ -23,6 +23,7 @@
> > >
> > > #include "crypto/cipher.h"
> > > #include "crypto/ivgen.h"
> > > +#include "block/block.h"
> > >
> > > typedef struct QCryptoBlock QCryptoBlock;
> > >
> > > @@ -268,4 +269,30 @@ uint64_t qcrypto_block_get_sector_size(QCryptoBlock
> > > *block);
> > > */
> > > void qcrypto_block_free(QCryptoBlock *block);
> > >
> > > +
> > > +/**
> > > + * qcrypto_block_setup_encryption:
> > > + * @block: the block encryption object
> > > + *
> > > + * @readfunc: callback for reading data from the volume header
> > > + * @writefunc: callback for writing data to the volume header
> > > + * @opaque: data to pass to @readfunc and @writefunc
> > > + * @action: tell the driver the setup action (add/erase currently)
> > > + * @options: driver specific options, that specify
> > > + * what encryption settings to manage
> > > + * @force: hint for the driver to allow unsafe operation
> > > + * @errp: error pointer
> > > + *
> > > + * Adds/Erases a new encryption key using @options
> >
> > I'd prefer to see separate APIs for add + erase instead
> > of overloading. It'll lead to a clearer API from callers
> > POV to see exactly which parameters are for each action.
>
>
> The downside of this is some duplication of code in the middle layers.
> I don't mind doing that if you are OK with that.
>
> Also note that if we go with amend options, we will have to use the single
> API.
That should not be the case. The interface used at the qemu-img/QMP
layer is something for the block layer to be concerned with. THis
code is in the crypto layer and shouldn't depend on stuff defined
in the block layer QAPI schema. The block layer ammend options impl
should translate to the relevant API calls needed at the crypto layer.
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 :|
