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

2018-03-12 Thread Daniel P . Berrangé 
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 
> > > ---
> > >  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, 
> > > _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
> _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é 

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

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

2018-03-12 Thread Kevin Wolf 
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 
> > ---
> >  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, 
> > _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
_crypto_create_opts_luks), so implementing that is matter for a
different patch.

Kevin

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

2018-03-12 Thread Daniel P . Berrangé 
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 
> ---
>  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, 
> _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.

> 
>  
> @@ -333,11 +317,10 @@ static int 
> block_crypto_create_generic(QCryptoBlockFormat format,
>  struct BlockCryptoCreateData data = {
>  .size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
>   BDRV_SECTOR_SIZE),
> -.opts = opts,
> -.filename = filename,
>  };
>  QDict *cryptoopts;
>  
> +/* Parse options */
>  cryptoopts = qemu_opts_to_qdict(opts, NULL);
>  
>  create_opts = block_crypto_create_opts_init(format, cryptoopts, errp);
> @@ -345,6 +328,20 @@ static int 
> block_crypto_create_generic(QCryptoBlockFormat format,
>  return -1;
>  }
>  
> +/* Create protocol layer */
> +ret = bdrv_create_file(filename, opts, errp);
> +if (ret < 0) {
> +return ret;
> +}
> +
> +data.blk = blk_new_open(filename, NULL, NULL,
> +BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
> +errp);
> +if (!data.blk) {
> +return -EINVAL;
> +}
> +
> +/* Create format layer */
>  crypto = qcrypto_block_create(create_opts, NULL,
>block_crypto_init_func,
>block_crypto_write_func,
> -- 
> 2.13.6
> 

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

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

2018-03-09 Thread Eric Blake 

On 03/09/2018 11:27 AM, 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 
---
  block/crypto.c | 37 +
  1 file changed, 17 insertions(+), 20 deletions(-)



Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

2018-03-09 Thread Kevin Wolf 
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 
---
 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, _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);
 }
 
 
@@ -333,11 +317,10 @@ static int block_crypto_create_generic(QCryptoBlockFormat 
format,
 struct BlockCryptoCreateData data = {
 .size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
  BDRV_SECTOR_SIZE),
-.opts = opts,
-.filename = filename,
 };
 QDict *cryptoopts;
 
+/* Parse options */
 cryptoopts = qemu_opts_to_qdict(opts, NULL);
 
 create_opts = block_crypto_create_opts_init(format, cryptoopts, errp);
@@ -345,6 +328,20 @@ static int block_crypto_create_generic(QCryptoBlockFormat 
format,
 return -1;
 }
 
+/* Create protocol layer */
+ret = bdrv_create_file(filename, opts, errp);
+if (ret < 0) {
+return ret;
+}
+
+data.blk = blk_new_open(filename, NULL, NULL,
+BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
+errp);
+if (!data.blk) {
+return -EINVAL;
+}
+
+/* Create format layer */
 crypto = qcrypto_block_create(create_opts, NULL,
   block_crypto_init_func,
   block_crypto_write_func,
-- 
2.13.6