diff mbox series

[1/6] luks: Separate image file creation from formatting

Message ID 20180309172713.26318-2-kwolf@redhat.com
State New
Headers show
Series luks: Implement .bdrv_co_create | expand

Commit Message

Kevin Wolf March 9, 2018, 5:27 p.m. UTC 
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(-)

Comments

Eric Blake March 9, 2018, 7:40 p.m. UTC | #1 
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 <kwolf@redhat.com>
> ---
>   block/crypto.c | 37 +++++++++++++++++--------------------
>   1 file changed, 17 insertions(+), 20 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>
Daniel P. Berrangé March 12, 2018, 11:35 a.m. UTC | #2 
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.

> 
>  
> @@ -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
Kevin Wolf March 12, 2018, 11:43 a.m. UTC | #3 
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.

Kevin
Daniel P. Berrangé March 12, 2018, 11:46 a.m. UTC | #4 
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
diff mbox series

Patch

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);
 }
 
 
@@ -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,