| Message ID | 20180309172713.26318-4-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | luks: Implement .bdrv_co_create | expand |
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 <kwolf@redhat.com> > --- > 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 <eblake@redhat.com>
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 <kwolf@redhat.com> > --- > 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 = &create_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, &create_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 = &block_crypto_create_opts_luks, > -- > 2.13.6 > Regards, Daniel
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 <kwolf@redhat.com> > > --- > > 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
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 <kwolf@redhat.com> > > > --- > > > 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
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 <kwolf@redhat.com> > > > > --- > > > > 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
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.
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 = &create_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, &create_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 = &block_crypto_create_opts_luks,
This adds the .bdrv_co_create driver callback to luks, which enables image creation over QMP. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-core.json | 17 ++++++++++++++++- block/crypto.c | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-)