Re: [Qemu-devel] [PATCH 3/6] luks: Support .bdrv_co_create

[Eric Blake]

On 03/12/2018 06:47 AM, Kevin Wolf wrote:


+##
+{ 'struct': 'BlockdevCreateOptionsLUKS',
+  'data': { 'file': 'BlockdevRef',
+'qcrypto':  'QCryptoBlockCreateOptionsLUKS',
+'size': 'size' } }


s/qcrypto/crypto/ in this field.

I do wonder about whether instead of embedding QCryptoBlockCreateOptionsLUKS
as a field, we could instead use QCryptoBlockCreateOptionsLUKS as the
base struct and just inherit from it to add file/size.

I'm not really fussed, but I wonder if you/Eric have any thoughts on the pros
or cons of inheritance vs embedding in this case.


I don't think QAPI has a way to embed it in JSON, but still provide a
QCryptoBlockCreateOptionsLUKS C object. Originally I wanted to use
inheritance, but instead of having the struct type reused, you get all
field definitions copied. I guess you could then manually cast to the
base type and it would still work, but it didn't really feel clean.


The QAPI generators generate a function (qapi_NAME_base()) which will 
convert any derived type back to the parent class, which is cleaner than 
manually casting; if that makes the decision to use inheritance any easier.


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

Re: [Qemu-devel] [PATCH 3/6] luks: Support .bdrv_co_create

[Kevin Wolf]

Am 12.03.2018 um 12:50 hat Daniel P. Berrangé geschrieben:
> On Mon, Mar 12, 2018 at 12:47:21PM +0100, Kevin Wolf wrote:
> > Am 12.03.2018 um 12:40 hat Daniel P. Berrangé geschrieben:
> > > On Fri, Mar 09, 2018 at 06:27:10PM +0100, Kevin Wolf wrote:
> > > > This adds the .bdrv_co_create driver callback to luks, which enables
> > > > image creation over QMP.
> > > > 
> > > > Signed-off-by: Kevin Wolf 
> > > > ---
> > > >  qapi/block-core.json | 17 -
> > > >  block/crypto.c   | 34 ++
> > > >  2 files changed, 50 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > > index 524d51567a..07039bfe9c 100644
> > > > --- a/qapi/block-core.json
> > > > +++ b/qapi/block-core.json
> > > > @@ -3452,6 +3452,21 @@
> > > >  '*preallocation':   'PreallocMode' } }
> > > >  
> > > >  ##
> > > > +# @BlockdevCreateOptionsLUKS:
> > > > +#
> > > > +# Driver specific image creation options for LUKS.
> > > > +#
> > > > +# @file Node to create the image format on
> > > > +# @size Size of the virtual disk in bytes
> > > > +#
> > > > +# Since: 2.12
> > > > +##
> > > > +{ 'struct': 'BlockdevCreateOptionsLUKS',
> > > > +  'data': { 'file': 'BlockdevRef',
> > > > +'qcrypto':  'QCryptoBlockCreateOptionsLUKS',
> > > > +'size': 'size' } }
> > > 
> > > s/qcrypto/crypto/ in this field.
> > > 
> > > I do wonder about whether instead of embedding 
> > > QCryptoBlockCreateOptionsLUKS
> > > as a field, we could instead use QCryptoBlockCreateOptionsLUKS as the
> > > base struct and just inherit from it to add file/size.
> > > 
> > > I'm not really fussed, but I wonder if you/Eric have any thoughts on the 
> > > pros
> > > or cons of inheritance vs embedding in this case.
> > 
> > I don't think QAPI has a way to embed it in JSON, but still provide a
> > QCryptoBlockCreateOptionsLUKS C object. Originally I wanted to use
> > inheritance, but instead of having the struct type reused, you get all
> > field definitions copied. I guess you could then manually cast to the
> > base type and it would still work, but it didn't really feel clean.
> 
> IIRC, QAPI generates a method
> 
>$BASETYPE *qapi_$TYPENAME_base(const $TYPENAME *obj)
> 
> which just does the blind cast to the base type.

Oh, thanks, I didn't know that one! That makes it an offical API that
the QAPI generators must keep working, so it should be fine to use. I'll
give it a try then.

Kevin

Re: [Qemu-devel] [PATCH 3/6] luks: Support .bdrv_co_create

[Daniel P . Berrangé]

On Mon, Mar 12, 2018 at 12:47:21PM +0100, Kevin Wolf wrote:
> Am 12.03.2018 um 12:40 hat Daniel P. Berrangé geschrieben:
> > On Fri, Mar 09, 2018 at 06:27:10PM +0100, Kevin Wolf wrote:
> > > This adds the .bdrv_co_create driver callback to luks, which enables
> > > image creation over QMP.
> > > 
> > > Signed-off-by: Kevin Wolf 
> > > ---
> > >  qapi/block-core.json | 17 -
> > >  block/crypto.c   | 34 ++
> > >  2 files changed, 50 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > index 524d51567a..07039bfe9c 100644
> > > --- a/qapi/block-core.json
> > > +++ b/qapi/block-core.json
> > > @@ -3452,6 +3452,21 @@
> > >  '*preallocation':   'PreallocMode' } }
> > >  
> > >  ##
> > > +# @BlockdevCreateOptionsLUKS:
> > > +#
> > > +# Driver specific image creation options for LUKS.
> > > +#
> > > +# @file Node to create the image format on
> > > +# @size Size of the virtual disk in bytes
> > > +#
> > > +# Since: 2.12
> > > +##
> > > +{ 'struct': 'BlockdevCreateOptionsLUKS',
> > > +  'data': { 'file': 'BlockdevRef',
> > > +'qcrypto':  'QCryptoBlockCreateOptionsLUKS',
> > > +'size': 'size' } }
> > 
> > s/qcrypto/crypto/ in this field.
> > 
> > I do wonder about whether instead of embedding QCryptoBlockCreateOptionsLUKS
> > as a field, we could instead use QCryptoBlockCreateOptionsLUKS as the
> > base struct and just inherit from it to add file/size.
> > 
> > I'm not really fussed, but I wonder if you/Eric have any thoughts on the 
> > pros
> > or cons of inheritance vs embedding in this case.
> 
> I don't think QAPI has a way to embed it in JSON, but still provide a
> QCryptoBlockCreateOptionsLUKS C object. Originally I wanted to use
> inheritance, but instead of having the struct type reused, you get all
> field definitions copied. I guess you could then manually cast to the
> base type and it would still work, but it didn't really feel clean.

IIRC, QAPI generates a method

   $BASETYPE *qapi_$TYPENAME_base(const $TYPENAME *obj)

which just does the blind cast to the base type.

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 3/6] luks: Support .bdrv_co_create

[Kevin Wolf]

Am 12.03.2018 um 12:40 hat Daniel P. Berrangé geschrieben:
> On Fri, Mar 09, 2018 at 06:27:10PM +0100, Kevin Wolf wrote:
> > This adds the .bdrv_co_create driver callback to luks, which enables
> > image creation over QMP.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  qapi/block-core.json | 17 -
> >  block/crypto.c   | 34 ++
> >  2 files changed, 50 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 524d51567a..07039bfe9c 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -3452,6 +3452,21 @@
> >  '*preallocation':   'PreallocMode' } }
> >  
> >  ##
> > +# @BlockdevCreateOptionsLUKS:
> > +#
> > +# Driver specific image creation options for LUKS.
> > +#
> > +# @file Node to create the image format on
> > +# @size Size of the virtual disk in bytes
> > +#
> > +# Since: 2.12
> > +##
> > +{ 'struct': 'BlockdevCreateOptionsLUKS',
> > +  'data': { 'file': 'BlockdevRef',
> > +'qcrypto':  'QCryptoBlockCreateOptionsLUKS',
> > +'size': 'size' } }
> 
> s/qcrypto/crypto/ in this field.
> 
> I do wonder about whether instead of embedding QCryptoBlockCreateOptionsLUKS
> as a field, we could instead use QCryptoBlockCreateOptionsLUKS as the
> base struct and just inherit from it to add file/size.
> 
> I'm not really fussed, but I wonder if you/Eric have any thoughts on the pros
> or cons of inheritance vs embedding in this case.

I don't think QAPI has a way to embed it in JSON, but still provide a
QCryptoBlockCreateOptionsLUKS C object. Originally I wanted to use
inheritance, but instead of having the struct type reused, you get all
field definitions copied. I guess you could then manually cast to the
base type and it would still work, but it didn't really feel clean.

Kevin

Re: [Qemu-devel] [PATCH 3/6] luks: Support .bdrv_co_create

[Daniel P . Berrangé]

On Fri, Mar 09, 2018 at 06:27:10PM +0100, Kevin Wolf wrote:
> This adds the .bdrv_co_create driver callback to luks, which enables
> image creation over QMP.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  qapi/block-core.json | 17 -
>  block/crypto.c   | 34 ++
>  2 files changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 524d51567a..07039bfe9c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3452,6 +3452,21 @@
>  '*preallocation':   'PreallocMode' } }
>  
>  ##
> +# @BlockdevCreateOptionsLUKS:
> +#
> +# Driver specific image creation options for LUKS.
> +#
> +# @file Node to create the image format on
> +# @size Size of the virtual disk in bytes
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'BlockdevCreateOptionsLUKS',
> +  'data': { 'file': 'BlockdevRef',
> +'qcrypto':  'QCryptoBlockCreateOptionsLUKS',
> +'size': 'size' } }

s/qcrypto/crypto/ in this field.


I do wonder about whether instead of embedding QCryptoBlockCreateOptionsLUKS
as a field, we could instead use QCryptoBlockCreateOptionsLUKS as the
base struct and just inherit from it to add file/size.

I'm not really fussed, but I wonder if you/Eric have any thoughts on the pros
or cons of inheritance vs embedding in this case.

> +
> +##
>  # @BlockdevCreateOptionsNfs:
>  #
>  # Driver specific image creation options for NFS.
> @@ -3643,7 +3658,7 @@
>'http':   'BlockdevCreateNotSupported',
>'https':  'BlockdevCreateNotSupported',
>'iscsi':  'BlockdevCreateNotSupported',
> -  'luks':   'BlockdevCreateNotSupported',
> +  'luks':   'BlockdevCreateOptionsLUKS',
>'nbd':'BlockdevCreateNotSupported',
>'nfs':'BlockdevCreateOptionsNfs',
>'null-aio':   'BlockdevCreateNotSupported',
> diff --git a/block/crypto.c b/block/crypto.c
> index b0a4cb3388..2035f9ab13 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -543,6 +543,39 @@ static int block_crypto_open_luks(BlockDriverState *bs,
>   bs, options, flags, errp);
>  }
>  
> +static int coroutine_fn
> +block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error 
> **errp)
> +{
> +BlockdevCreateOptionsLUKS *luks_opts;
> +BlockDriverState *bs = NULL;
> +QCryptoBlockCreateOptions create_opts;
> +int ret;
> +
> +assert(create_options->driver == BLOCKDEV_DRIVER_LUKS);
> +luks_opts = _options->u.luks;
> +
> +bs = bdrv_open_blockdev_ref(luks_opts->file, errp);
> +if (bs == NULL) {
> +return -EIO;
> +}
> +
> +create_opts = (QCryptoBlockCreateOptions) {
> +.format = Q_CRYPTO_BLOCK_FORMAT_LUKS,
> +.u.luks = *luks_opts->qcrypto,
> +};
> +
> +ret = block_crypto_co_create_generic(bs, luks_opts->size, _opts,
> + errp);
> +if (ret < 0) {
> +goto fail;
> +}
> +
> +ret = 0;
> +fail:
> +bdrv_unref(bs);
> +return ret;
> +}
> +
>  static int coroutine_fn block_crypto_co_create_opts_luks(const char 
> *filename,
>   QemuOpts *opts,
>   Error **errp)
> @@ -647,6 +680,7 @@ BlockDriver bdrv_crypto_luks = {
>  .bdrv_open  = block_crypto_open_luks,
>  .bdrv_close = block_crypto_close,
>  .bdrv_child_perm= bdrv_format_default_perms,
> +.bdrv_co_create = block_crypto_co_create_luks,
>  .bdrv_co_create_opts = block_crypto_co_create_opts_luks,
>  .bdrv_truncate  = block_crypto_truncate,
>  .create_opts= _crypto_create_opts_luks,
> -- 
> 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 3/6] luks: Support .bdrv_co_create

[Eric Blake]

On 03/09/2018 11:27 AM, Kevin Wolf wrote:

This adds the .bdrv_co_create driver callback to luks, which enables
image creation over QMP.

Signed-off-by: Kevin Wolf 
---
  qapi/block-core.json | 17 -
  block/crypto.c   | 34 ++
  2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 524d51567a..07039bfe9c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3452,6 +3452,21 @@
  '*preallocation':   'PreallocMode' } }
  
  ##

+# @BlockdevCreateOptionsLUKS:
+#
+# Driver specific image creation options for LUKS.
+#
+# @file Node to create the image format on
+# @size Size of the virtual disk in bytes
+#
+# Since: 2.12


Well, as long as we make it by Tuesday :)

Reviewed-by: Eric Blake 

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

[Qemu-devel] [PATCH 3/6] luks: Support .bdrv_co_create

[Kevin Wolf]

This adds the .bdrv_co_create driver callback to luks, which enables
image creation over QMP.

Signed-off-by: Kevin Wolf 
---
 qapi/block-core.json | 17 -
 block/crypto.c   | 34 ++
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 524d51567a..07039bfe9c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3452,6 +3452,21 @@
 '*preallocation':   'PreallocMode' } }
 
 ##
+# @BlockdevCreateOptionsLUKS:
+#
+# Driver specific image creation options for LUKS.
+#
+# @file Node to create the image format on
+# @size Size of the virtual disk in bytes
+#
+# Since: 2.12
+##
+{ 'struct': 'BlockdevCreateOptionsLUKS',
+  'data': { 'file': 'BlockdevRef',
+'qcrypto':  'QCryptoBlockCreateOptionsLUKS',
+'size': 'size' } }
+
+##
 # @BlockdevCreateOptionsNfs:
 #
 # Driver specific image creation options for NFS.
@@ -3643,7 +3658,7 @@
   'http':   'BlockdevCreateNotSupported',
   'https':  'BlockdevCreateNotSupported',
   'iscsi':  'BlockdevCreateNotSupported',
-  'luks':   'BlockdevCreateNotSupported',
+  'luks':   'BlockdevCreateOptionsLUKS',
   'nbd':'BlockdevCreateNotSupported',
   'nfs':'BlockdevCreateOptionsNfs',
   'null-aio':   'BlockdevCreateNotSupported',
diff --git a/block/crypto.c b/block/crypto.c
index b0a4cb3388..2035f9ab13 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -543,6 +543,39 @@ static int block_crypto_open_luks(BlockDriverState *bs,
  bs, options, flags, errp);
 }
 
+static int coroutine_fn
+block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error 
**errp)
+{
+BlockdevCreateOptionsLUKS *luks_opts;
+BlockDriverState *bs = NULL;
+QCryptoBlockCreateOptions create_opts;
+int ret;
+
+assert(create_options->driver == BLOCKDEV_DRIVER_LUKS);
+luks_opts = _options->u.luks;
+
+bs = bdrv_open_blockdev_ref(luks_opts->file, errp);
+if (bs == NULL) {
+return -EIO;
+}
+
+create_opts = (QCryptoBlockCreateOptions) {
+.format = Q_CRYPTO_BLOCK_FORMAT_LUKS,
+.u.luks = *luks_opts->qcrypto,
+};
+
+ret = block_crypto_co_create_generic(bs, luks_opts->size, _opts,
+ errp);
+if (ret < 0) {
+goto fail;
+}
+
+ret = 0;
+fail:
+bdrv_unref(bs);
+return ret;
+}
+
 static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename,
  QemuOpts *opts,
  Error **errp)
@@ -647,6 +680,7 @@ BlockDriver bdrv_crypto_luks = {
 .bdrv_open  = block_crypto_open_luks,
 .bdrv_close = block_crypto_close,
 .bdrv_child_perm= bdrv_format_default_perms,
+.bdrv_co_create = block_crypto_co_create_luks,
 .bdrv_co_create_opts = block_crypto_co_create_opts_luks,
 .bdrv_truncate  = block_crypto_truncate,
 .create_opts= _crypto_create_opts_luks,
-- 
2.13.6