Re: [Qemu-devel] [PATCH 1/6] luks: Separate image file creation from formatting

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Mar 12, 2018 at 12:43:47PM +0100, Kevin Wolf wrote:
> Am 12.03.2018 um 12:35 hat Daniel P. Berrangé geschrieben:
> > On Fri, Mar 09, 2018 at 06:27:08PM +0100, Kevin Wolf wrote:
> > > The crypto driver used to create the image file in a callback from the
> > > crypto subsystem. If we want to implement .bdrv_co_create, this needs to
> > > go away because that callback will get a reference to an already
> > > existing block node.
> > > 
> > > Move the image file creation to block_crypto_create_generic().
> > > 
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > >  block/crypto.c | 37 +++++++++++++++++--------------------
> > >  1 file changed, 17 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/block/crypto.c b/block/crypto.c
> > > index e6095e7807..77871640cc 100644
> > > --- a/block/crypto.c
> > > +++ b/block/crypto.c
> > > @@ -71,8 +71,6 @@ static ssize_t block_crypto_read_func(QCryptoBlock *block,
> > >  
> > >  
> > >  struct BlockCryptoCreateData {
> > > -    const char *filename;
> > > -    QemuOpts *opts;
> > >      BlockBackend *blk;
> > >      uint64_t size;
> > >  };
> > > @@ -103,27 +101,13 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block,
> > >                                        Error **errp)
> > >  {
> > >      struct BlockCryptoCreateData *data = opaque;
> > > -    int ret;
> > >  
> > >      /* User provided size should reflect amount of space made
> > >       * available to the guest, so we must take account of that
> > >       * which will be used by the crypto header
> > >       */
> > > -    data->size += headerlen;
> > > -
> > > -    qemu_opt_set_number(data->opts, BLOCK_OPT_SIZE, data->size, &error_abort);
> > > -    ret = bdrv_create_file(data->filename, data->opts, errp);
> > > -    if (ret < 0) {
> > > -        return -1;
> > > -    }
> > > -
> > > -    data->blk = blk_new_open(data->filename, NULL, NULL,
> > > -                             BDRV_O_RDWR | BDRV_O_PROTOCOL, errp);
> > > -    if (!data->blk) {
> > > -        return -1;
> > > -    }
> > > -
> > > -    return 0;
> > > +    return blk_truncate(data->blk, data->size + headerlen, PREALLOC_MODE_OFF,
> > > +                        errp);
> > >  }
> > 
> > I wonder if we should pass in the value of the PREALLOC_MODE from the
> > block_crypto_create_generic() function via BlockCryptoCreateData, so
> > that we honour it. By using PREALLOC_MODE_OFF here, all LUKS volumes
> > created with PREALLOC_MODE_FULL are going to have several MB of
> > unallocated extents at the end of the file.
> 
> That would be an easy way to implement preallocation support. But you
> can't create LUKS volumes with PREALLOC_MODE_FULL today because it
> doesn't support preallocation at all (there's no option for it in
> &block_crypto_create_opts_luks), so implementing that is matter for a
> different patch.

Hah, ops, I forgot that ! In that case,

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

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