[v2,09/11] block/qcow2: implement blockdev-amend
diff mbox series

Message ID 20190912223028.18496-10-mlevitsk@redhat.com
State New
Headers show
Series
  • RFC crypto/luks: encryption key managment using amend interface
Related show

Commit Message

Maxim Levitsky Sept. 12, 2019, 10:30 p.m. UTC
Currently only for changing crypto parameters

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block/qcow2.c        | 71 ++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json |  6 ++--
 2 files changed, 75 insertions(+), 2 deletions(-)

Comments

Max Reitz Oct. 4, 2019, 7:03 p.m. UTC | #1
On 13.09.19 00:30, Maxim Levitsky wrote:
> Currently only for changing crypto parameters

Yep, that elegantly avoids most of the problems we’d have otherwise. :-)

> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  block/qcow2.c        | 71 ++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json |  6 ++--
>  2 files changed, 75 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 26f83aeb44..c8847ec6e2 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3079,6 +3079,18 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
>      assert(create_options->driver == BLOCKDEV_DRIVER_QCOW2);
>      qcow2_opts = &create_options->u.qcow2;
>  
> +    if (!qcow2_opts->has_size) {
> +        error_setg(errp, "Size is manadatory for image creation");
> +        return -EINVAL;
> +
> +    }
> +
> +    if (!qcow2_opts->has_file) {
> +        error_setg(errp, "'file' is manadatory for image creation");
> +        return -EINVAL;
> +
> +    }
> +
>      bs = bdrv_open_blockdev_ref(qcow2_opts->file, errp);
>      if (bs == NULL) {
>          return -EIO;
> @@ -5111,6 +5123,64 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
>      return 0;
>  }
>  
> +
> +static int coroutine_fn qcow2_co_amend(BlockDriverState *bs,
> +                                       BlockdevCreateOptions *opts,
> +                                       bool force,
> +                                       Error **errp)
> +{
> +    BlockdevCreateOptionsQcow2 *qopts = &opts->u.qcow2;
> +    BDRVQcow2State *s = bs->opaque;
> +    int ret;
> +
> +    /*
> +     * This is ugly as hell, in later versions of this patch
> +     * something has to be done about this

Well, at least the language of the comment needs adjustment. :-)

> +     */
> +    if (qopts->has_file || qopts->has_size || qopts->has_data_file ||
> +        qopts->has_data_file_raw || qopts->has_version ||
> +        qopts->has_backing_file || qopts->has_backing_fmt ||
> +        qopts->has_cluster_size || qopts->has_preallocation ||
> +        qopts->has_lazy_refcounts || qopts->has_refcount_bits) {
> +
> +        error_setg(errp,
> +                "Only LUKS encryption options can be amended for qcow2 with blockdev-amend");
> +        return -EOPNOTSUPP;
> +
> +    }
> +
> +    if (qopts->has_encrypt) {
> +        if (!s->crypto) {
> +            error_setg(errp, "QCOW2 image is not encrypted, can't amend");
> +            return -EOPNOTSUPP;
> +        }
> +
> +        if (qopts->encrypt->format != Q_CRYPTO_BLOCK_FORMAT_LUKS) {
> +            error_setg(errp,
> +                       "Amend can't be used to change the qcow2 encryption format");
> +            return -EOPNOTSUPP;
> +        }
> +
> +        if (s->crypt_method_header != QCOW_CRYPT_LUKS) {
> +            error_setg(errp,
> +                       "Only LUKS encryption options can be amended for qcow2 with blockdev-amend");
> +            return -EOPNOTSUPP;
> +        }
> +
> +        ret = qcrypto_block_amend_options(s->crypto,
> +                                          qcow2_crypto_hdr_read_func,
> +                                          qcow2_crypto_hdr_write_func,
> +                                          bs,
> +                                          qopts->encrypt,
> +                                          force,
> +                                          errp);
> +        if (ret) {
> +            return ret;
> +        }
> +    }
> +    return 0;

I suppose we need to do the same permission stuff as for LUKS, though;
i.e., unshare CONSISTENT_READ while this operation is ongoing.

> +}
> +
>  /*
>   * If offset or size are negative, respectively, they will not be included in
>   * the BLOCK_IMAGE_CORRUPTED event emitted.
> @@ -5303,6 +5373,7 @@ BlockDriver bdrv_qcow2 = {
>      .mutable_opts        = mutable_opts,
>      .bdrv_co_check       = qcow2_co_check,
>      .bdrv_amend_options  = qcow2_amend_options,
> +    .bdrv_co_amend       = qcow2_co_amend,
>  
>      .bdrv_detach_aio_context  = qcow2_detach_aio_context,
>      .bdrv_attach_aio_context  = qcow2_attach_aio_context,
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 4a6db98938..0eb4e45168 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4294,6 +4294,7 @@
>  # Driver specific image creation options for qcow2.
>  #
>  # @file             Node to create the image format on
> +#                   Mandatory for create
>  # @data-file        Node to use as an external data file in which all guest
>  #                   data is stored so that only metadata remains in the qcow2
>  #                   file (since: 4.0)
> @@ -4301,6 +4302,7 @@
>  #                   standalone (read-only) raw image without looking at qcow2
>  #                   metadata (default: false; since: 4.0)
>  # @size             Size of the virtual disk in bytes
> +#                   Mandatory for create
>  # @version          Compatibility level (default: v3)
>  # @backing-file     File name of the backing file if a backing file
>  #                   should be used
> @@ -4315,10 +4317,10 @@
>  # Since: 2.12
>  ##
>  { 'struct': 'BlockdevCreateOptionsQcow2',
> -  'data': { 'file':             'BlockdevRef',
> +  'data': { '*file':            'BlockdevRef',
>              '*data-file':       'BlockdevRef',
>              '*data-file-raw':   'bool',
> -            'size':             'size',
> +            '*size':            'size',
>              '*version':         'BlockdevQcow2Version',
>              '*backing-file':    'str',
>              '*backing-fmt':     'BlockdevDriver',
> 

Making these fields optional makes me wonder whether it actually make
sense to have both create and amend share a single QAPI structure.
Wouldn’t it better to have a separate amend structure that then actually
reflects what we support?  (This would also solve the problem of how to
express in the code what is and what isn’t supported through
blockdev-amend.)

Max
Markus Armbruster Oct. 7, 2019, 8:04 a.m. UTC | #2
Max Reitz <mreitz@redhat.com> writes:

> On 13.09.19 00:30, Maxim Levitsky wrote:
>> Currently only for changing crypto parameters
>
> Yep, that elegantly avoids most of the problems we’d have otherwise. :-)
>
>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
[...]
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 4a6db98938..0eb4e45168 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -4294,6 +4294,7 @@
>>  # Driver specific image creation options for qcow2.
>>  #
>>  # @file             Node to create the image format on
>> +#                   Mandatory for create
>>  # @data-file        Node to use as an external data file in which all guest
>>  #                   data is stored so that only metadata remains in the qcow2
>>  #                   file (since: 4.0)
>> @@ -4301,6 +4302,7 @@
>>  #                   standalone (read-only) raw image without looking at qcow2
>>  #                   metadata (default: false; since: 4.0)
>>  # @size             Size of the virtual disk in bytes
>> +#                   Mandatory for create
>>  # @version          Compatibility level (default: v3)
>>  # @backing-file     File name of the backing file if a backing file
>>  #                   should be used
>> @@ -4315,10 +4317,10 @@
>>  # Since: 2.12
>>  ##
>>  { 'struct': 'BlockdevCreateOptionsQcow2',
>> -  'data': { 'file':             'BlockdevRef',
>> +  'data': { '*file':            'BlockdevRef',
>>              '*data-file':       'BlockdevRef',
>>              '*data-file-raw':   'bool',
>> -            'size':             'size',
>> +            '*size':            'size',
>>              '*version':         'BlockdevQcow2Version',
>>              '*backing-file':    'str',
>>              '*backing-fmt':     'BlockdevDriver',
>> 

My comments to the previous patch apply.

> Making these fields optional makes me wonder whether it actually make
> sense to have both create and amend share a single QAPI structure.
> Wouldn’t it better to have a separate amend structure that then actually
> reflects what we support?  (This would also solve the problem of how to
> express in the code what is and what isn’t supported through
> blockdev-amend.)

Good point.  As is, the schema is rather confusing, at least to me.  We
reduce or avoid the confusion in documentation or in code.  I'd prefer
code unless it leads to too much duplication.  "Too much" is of course
subjective.  Maxim, would you mind exploring it for us?

Patch
diff mbox series

diff --git a/block/qcow2.c b/block/qcow2.c
index 26f83aeb44..c8847ec6e2 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3079,6 +3079,18 @@  qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
     assert(create_options->driver == BLOCKDEV_DRIVER_QCOW2);
     qcow2_opts = &create_options->u.qcow2;
 
+    if (!qcow2_opts->has_size) {
+        error_setg(errp, "Size is manadatory for image creation");
+        return -EINVAL;
+
+    }
+
+    if (!qcow2_opts->has_file) {
+        error_setg(errp, "'file' is manadatory for image creation");
+        return -EINVAL;
+
+    }
+
     bs = bdrv_open_blockdev_ref(qcow2_opts->file, errp);
     if (bs == NULL) {
         return -EIO;
@@ -5111,6 +5123,64 @@  static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
     return 0;
 }
 
+
+static int coroutine_fn qcow2_co_amend(BlockDriverState *bs,
+                                       BlockdevCreateOptions *opts,
+                                       bool force,
+                                       Error **errp)
+{
+    BlockdevCreateOptionsQcow2 *qopts = &opts->u.qcow2;
+    BDRVQcow2State *s = bs->opaque;
+    int ret;
+
+    /*
+     * This is ugly as hell, in later versions of this patch
+     * something has to be done about this
+     */
+    if (qopts->has_file || qopts->has_size || qopts->has_data_file ||
+        qopts->has_data_file_raw || qopts->has_version ||
+        qopts->has_backing_file || qopts->has_backing_fmt ||
+        qopts->has_cluster_size || qopts->has_preallocation ||
+        qopts->has_lazy_refcounts || qopts->has_refcount_bits) {
+
+        error_setg(errp,
+                "Only LUKS encryption options can be amended for qcow2 with blockdev-amend");
+        return -EOPNOTSUPP;
+
+    }
+
+    if (qopts->has_encrypt) {
+        if (!s->crypto) {
+            error_setg(errp, "QCOW2 image is not encrypted, can't amend");
+            return -EOPNOTSUPP;
+        }
+
+        if (qopts->encrypt->format != Q_CRYPTO_BLOCK_FORMAT_LUKS) {
+            error_setg(errp,
+                       "Amend can't be used to change the qcow2 encryption format");
+            return -EOPNOTSUPP;
+        }
+
+        if (s->crypt_method_header != QCOW_CRYPT_LUKS) {
+            error_setg(errp,
+                       "Only LUKS encryption options can be amended for qcow2 with blockdev-amend");
+            return -EOPNOTSUPP;
+        }
+
+        ret = qcrypto_block_amend_options(s->crypto,
+                                          qcow2_crypto_hdr_read_func,
+                                          qcow2_crypto_hdr_write_func,
+                                          bs,
+                                          qopts->encrypt,
+                                          force,
+                                          errp);
+        if (ret) {
+            return ret;
+        }
+    }
+    return 0;
+}
+
 /*
  * If offset or size are negative, respectively, they will not be included in
  * the BLOCK_IMAGE_CORRUPTED event emitted.
@@ -5303,6 +5373,7 @@  BlockDriver bdrv_qcow2 = {
     .mutable_opts        = mutable_opts,
     .bdrv_co_check       = qcow2_co_check,
     .bdrv_amend_options  = qcow2_amend_options,
+    .bdrv_co_amend       = qcow2_co_amend,
 
     .bdrv_detach_aio_context  = qcow2_detach_aio_context,
     .bdrv_attach_aio_context  = qcow2_attach_aio_context,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4a6db98938..0eb4e45168 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4294,6 +4294,7 @@ 
 # Driver specific image creation options for qcow2.
 #
 # @file             Node to create the image format on
+#                   Mandatory for create
 # @data-file        Node to use as an external data file in which all guest
 #                   data is stored so that only metadata remains in the qcow2
 #                   file (since: 4.0)
@@ -4301,6 +4302,7 @@ 
 #                   standalone (read-only) raw image without looking at qcow2
 #                   metadata (default: false; since: 4.0)
 # @size             Size of the virtual disk in bytes
+#                   Mandatory for create
 # @version          Compatibility level (default: v3)
 # @backing-file     File name of the backing file if a backing file
 #                   should be used
@@ -4315,10 +4317,10 @@ 
 # Since: 2.12
 ##
 { 'struct': 'BlockdevCreateOptionsQcow2',
-  'data': { 'file':             'BlockdevRef',
+  'data': { '*file':            'BlockdevRef',
             '*data-file':       'BlockdevRef',
             '*data-file-raw':   'bool',
-            'size':             'size',
+            '*size':            'size',
             '*version':         'BlockdevQcow2Version',
             '*backing-file':    'str',
             '*backing-fmt':     'BlockdevDriver',