Re: [PATCH v2 02/11] qcrypto-luks: extend the create options for upcoming encryption key management

From: Maxim Levitsky <mlevitsk@redhat.com>
To: Max Reitz <mreitz@redhat.com>, qemu-devel@nongnu.org
Cc: "Kevin Wolf" <kwolf@redhat.com>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	qemu-block@nongnu.org, "Markus Armbruster" <armbru@redhat.com>,
	"John Snow" <jsnow@redhat.com>
Subject: Re: [PATCH v2 02/11] qcrypto-luks: extend the create options for upcoming encryption key management
Date: Fri, 08 Nov 2019 13:48:46 +0200	[thread overview]
Message-ID: <8f0857a01eec965ac91fb44083227d7b9fe866f1.camel@redhat.com> (raw)
In-Reply-To: <af4b3495-0b8d-e269-4190-779535526ab4@redhat.com>

On Fri, 2019-11-08 at 11:48 +0100, Max Reitz wrote:
> On 08.11.19 10:28, Maxim Levitsky wrote:
> > On Fri, 2019-10-04 at 19:42 +0200, Max Reitz wrote:
> > > On 13.09.19 00:30, Maxim Levitsky wrote:
> > > > Now you can specify which slot to put the encryption key to
> > > > Plus add 'active' option which will let  user erase the key secret
> > > > instead of adding it.
> > > > Check that active=true it when creating.
> > > > 
> > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > > ---
> > > >  block/crypto.c             |  2 ++
> > > >  block/crypto.h             | 16 +++++++++++
> > > >  block/qcow2.c              |  2 ++
> > > >  crypto/block-luks.c        | 26 +++++++++++++++---
> > > >  qapi/crypto.json           | 19 ++++++++++++++
> > > >  tests/qemu-iotests/082.out | 54 ++++++++++++++++++++++++++++++++++++++
> > > >  6 files changed, 115 insertions(+), 4 deletions(-)
> > > 
> > > (Just doing a cursory RFC-style review)
> > > 
> > > I think we also want to reject unlock-secret if it’s given for creation;
> > 
> > Agree, I'll do this in the next version.
> > 
> > > and I suppose it’d be more important to print which slots are OK than
> > > the slot the user has given.  (It isn’t like we shouldn’t print that
> > > slot index, but it’s more likely the user knows that than what the
> > > limits are.  I think.)
> > 
> > I don't really understand what you mean here :-( 
> > 
> > Since this is qmp interface,
> > I can't really print anything from it, other that error messages.
> 
> Exactly, I’m referring to the error message.  Right now it’s:
> 
> "Invalid slot %" PRId64 " is specified", luks_opts.slot
> 
> I think it should be something like:
> 
> "Invalid slot %" PRId64 " specified, must be between 0 and %u",
> luks_opt.slot, QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS - 1

This is a very good idea! implemented now and will
post in the next version.

Best regards,
	Maxim Levitsky