Re: [PATCH v2 09/11] block/qcow2: implement blockdev-amend

[Max Reitz]

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 <address@hidden>
> ---
>  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

signature.asc
Description: OpenPGP digital signature