[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 09/11] block/qcow2: implement blockdev-amend
From: |
Max Reitz |
Subject: |
Re: [PATCH v2 09/11] block/qcow2: implement blockdev-amend |
Date: |
Fri, 4 Oct 2019 21:03:16 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.0 |
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
- Re: [PATCH v2 09/11] block/qcow2: implement blockdev-amend,
Max Reitz <=