qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: "Daniel P. Berrangé" <berrange@redhat.com>
To: Maxim Levitsky <mlevitsk@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>,
	qemu-block@nongnu.org, qemu-devel@nongnu.org,
	Max Reitz <mreitz@redhat.com>, John Snow <jsnow@redhat.com>,
	Markus Armbruster <armbru@redhat.com>
Subject: Re: [PATCH 04/13] block: amend: separate amend and create options for qemu-img
Date: Tue, 28 Jan 2020 17:23:43 +0000	[thread overview]
Message-ID: <20200128172343.GW1446339@redhat.com> (raw)
In-Reply-To: <20200114193350.10830-5-mlevitsk@redhat.com>

On Tue, Jan 14, 2020 at 09:33:41PM +0200, Maxim Levitsky wrote:
> Some options are only useful for creation
> (or hard to be amended, like cluster size for qcow2), while some other
> options are only useful for amend, like upcoming keyslot management
> options for luks
> 
> Since currently only qcow2 supports amend, move all its options
> to a common macro and then include it in each action option list.
> 
> In future it might be useful to remove some options which are
> not supported anyway from amend list, which currently
> cause an error message if amended.

I think I would have done that in this commit. At least the
encrypt.* options shouldn't be added to the amend_opts list,
since they're being removed from it again a few patches later.

> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  block/qcow2.c             | 160 +++++++++++++++++++++-----------------
>  include/block/block_int.h |   4 +
>  qemu-img.c                |  18 ++---
>  3 files changed, 100 insertions(+), 82 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 6bcf4a5fc4..c6c2deee75 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -5445,83 +5445,96 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
>      s->signaled_corruption = true;
>  }
>  
> +#define QCOW_COMMON_OPTIONS                                         \
> +    {                                                               \
> +        .name = BLOCK_OPT_SIZE,                                     \
> +        .type = QEMU_OPT_SIZE,                                      \
> +        .help = "Virtual disk size"                                 \
> +    },                                                              \
> +    {                                                               \
> +        .name = BLOCK_OPT_COMPAT_LEVEL,                             \
> +        .type = QEMU_OPT_STRING,                                    \
> +        .help = "Compatibility level (v2 [0.10] or v3 [1.1])"       \
> +    },                                                              \
> +    {                                                               \
> +        .name = BLOCK_OPT_BACKING_FILE,                             \
> +        .type = QEMU_OPT_STRING,                                    \
> +        .help = "File name of a base image"                         \
> +    },                                                              \
> +    {                                                               \
> +        .name = BLOCK_OPT_BACKING_FMT,                              \
> +        .type = QEMU_OPT_STRING,                                    \
> +        .help = "Image format of the base image"                    \
> +    },                                                              \
> +    {                                                               \
> +        .name = BLOCK_OPT_DATA_FILE,                                \
> +        .type = QEMU_OPT_STRING,                                    \
> +        .help = "File name of an external data file"                \
> +    },                                                              \
> +    {                                                               \
> +        .name = BLOCK_OPT_DATA_FILE_RAW,                            \
> +        .type = QEMU_OPT_BOOL,                                      \
> +        .help = "The external data file must stay valid "           \
> +                "as a raw image"                                    \
> +    },                                                              \
> +    {                                                               \
> +        .name = BLOCK_OPT_ENCRYPT,                                  \
> +        .type = QEMU_OPT_BOOL,                                      \
> +        .help = "Encrypt the image with format 'aes'. (Deprecated " \
> +                "in favor of " BLOCK_OPT_ENCRYPT_FORMAT "=aes)",    \
> +    },                                                              \
> +    {                                                               \
> +        .name = BLOCK_OPT_ENCRYPT_FORMAT,                           \
> +        .type = QEMU_OPT_STRING,                                    \
> +        .help = "Encrypt the image, format choices: 'aes', 'luks'", \
> +    },                                                              \
> +    BLOCK_CRYPTO_OPT_DEF_KEY_SECRET("encrypt.",                     \
> +        "ID of secret providing qcow AES key or LUKS passphrase"),  \
> +    BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG("encrypt."),               \
> +    BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE("encrypt."),              \
> +    BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG("encrypt."),                \
> +    BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG("encrypt."),           \
> +    BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG("encrypt."),                 \
> +    BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME("encrypt."),                \
> +    {                                                               \
> +        .name = BLOCK_OPT_CLUSTER_SIZE,                             \
> +        .type = QEMU_OPT_SIZE,                                      \
> +        .help = "qcow2 cluster size",                               \
> +        .def_value_str = stringify(DEFAULT_CLUSTER_SIZE)            \
> +    },                                                              \
> +    {                                                               \
> +        .name = BLOCK_OPT_PREALLOC,                                 \
> +        .type = QEMU_OPT_STRING,                                    \
> +        .help = "Preallocation mode (allowed values: off, "         \
> +                "metadata, falloc, full)"                           \
> +    },                                                              \
> +    {                                                               \
> +        .name = BLOCK_OPT_LAZY_REFCOUNTS,                           \
> +        .type = QEMU_OPT_BOOL,                                      \
> +        .help = "Postpone refcount updates",                        \
> +        .def_value_str = "off"                                      \
> +    },                                                              \
> +    {                                                               \
> +        .name = BLOCK_OPT_REFCOUNT_BITS,                            \
> +        .type = QEMU_OPT_NUMBER,                                    \
> +        .help = "Width of a reference count entry in bits",         \
> +        .def_value_str = "16"                                       \
> +    }                                                               \
> +
>  static QemuOptsList qcow2_create_opts = {
>      .name = "qcow2-create-opts",
>      .head = QTAILQ_HEAD_INITIALIZER(qcow2_create_opts.head),
>      .desc = {
> -        {
> -            .name = BLOCK_OPT_SIZE,
> -            .type = QEMU_OPT_SIZE,
> -            .help = "Virtual disk size"
> -        },
> -        {
> -            .name = BLOCK_OPT_COMPAT_LEVEL,
> -            .type = QEMU_OPT_STRING,
> -            .help = "Compatibility level (v2 [0.10] or v3 [1.1])"
> -        },
> -        {
> -            .name = BLOCK_OPT_BACKING_FILE,
> -            .type = QEMU_OPT_STRING,
> -            .help = "File name of a base image"
> -        },
> -        {
> -            .name = BLOCK_OPT_BACKING_FMT,
> -            .type = QEMU_OPT_STRING,
> -            .help = "Image format of the base image"
> -        },
> -        {
> -            .name = BLOCK_OPT_DATA_FILE,
> -            .type = QEMU_OPT_STRING,
> -            .help = "File name of an external data file"
> -        },
> -        {
> -            .name = BLOCK_OPT_DATA_FILE_RAW,
> -            .type = QEMU_OPT_BOOL,
> -            .help = "The external data file must stay valid as a raw image"
> -        },
> -        {
> -            .name = BLOCK_OPT_ENCRYPT,
> -            .type = QEMU_OPT_BOOL,
> -            .help = "Encrypt the image with format 'aes'. (Deprecated "
> -                    "in favor of " BLOCK_OPT_ENCRYPT_FORMAT "=aes)",
> -        },
> -        {
> -            .name = BLOCK_OPT_ENCRYPT_FORMAT,
> -            .type = QEMU_OPT_STRING,
> -            .help = "Encrypt the image, format choices: 'aes', 'luks'",
> -        },
> -        BLOCK_CRYPTO_OPT_DEF_KEY_SECRET("encrypt.",
> -            "ID of secret providing qcow AES key or LUKS passphrase"),
> -        BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_ALG("encrypt."),
> -        BLOCK_CRYPTO_OPT_DEF_LUKS_CIPHER_MODE("encrypt."),
> -        BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_ALG("encrypt."),
> -        BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG("encrypt."),
> -        BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG("encrypt."),
> -        BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME("encrypt."),
> -        {
> -            .name = BLOCK_OPT_CLUSTER_SIZE,
> -            .type = QEMU_OPT_SIZE,
> -            .help = "qcow2 cluster size",
> -            .def_value_str = stringify(DEFAULT_CLUSTER_SIZE)
> -        },
> -        {
> -            .name = BLOCK_OPT_PREALLOC,
> -            .type = QEMU_OPT_STRING,
> -            .help = "Preallocation mode (allowed values: off, metadata, "
> -                    "falloc, full)"
> -        },
> -        {
> -            .name = BLOCK_OPT_LAZY_REFCOUNTS,
> -            .type = QEMU_OPT_BOOL,
> -            .help = "Postpone refcount updates",
> -            .def_value_str = "off"
> -        },
> -        {
> -            .name = BLOCK_OPT_REFCOUNT_BITS,
> -            .type = QEMU_OPT_NUMBER,
> -            .help = "Width of a reference count entry in bits",
> -            .def_value_str = "16"
> -        },
> +        QCOW_COMMON_OPTIONS,
> +        { /* end of list */ }
> +    }
> +};
> +
> +static QemuOptsList qcow2_amend_opts = {
> +    .name = "qcow2-amend-opts",
> +    .head = QTAILQ_HEAD_INITIALIZER(qcow2_amend_opts.head),
> +    .desc = {
> +        QCOW_COMMON_OPTIONS,
>          { /* end of list */ }
>      }
>  };
> @@ -5581,6 +5594,7 @@ BlockDriver bdrv_qcow2 = {
>      .bdrv_inactivate            = qcow2_inactivate,
>  
>      .create_opts         = &qcow2_create_opts,
> +    .amend_opts          = &qcow2_amend_opts,
>      .strong_runtime_opts = qcow2_strong_runtime_opts,
>      .mutable_opts        = mutable_opts,
>      .bdrv_co_check       = qcow2_co_check,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 810a9ecb86..6f0abf8544 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -407,6 +407,10 @@ struct BlockDriver {
>  
>      /* List of options for creating images, terminated by name == NULL */
>      QemuOptsList *create_opts;
> +
> +    /* List of options for image amend*/
> +    QemuOptsList *amend_opts;
> +
>      /*
>       * If this driver supports reopening images this contains a
>       * NULL-terminated list of the runtime options that can be
> diff --git a/qemu-img.c b/qemu-img.c
> index a79f3904db..befd53943b 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3878,11 +3878,11 @@ static int print_amend_option_help(const char *format)
>          return 1;
>      }
>  
> -    /* Every driver supporting amendment must have create_opts */
> -    assert(drv->create_opts);
> +    /* Every driver supporting amendment must have amend_opts */
> +    assert(drv->amend_opts);
>  
>      printf("Creation options for '%s':\n", format);
> -    qemu_opts_print_help(drv->create_opts, false);
> +    qemu_opts_print_help(drv->amend_opts, false);
>      printf("\nNote that not all of these options may be amendable.\n");
>      return 0;
>  }
> @@ -3892,7 +3892,7 @@ static int img_amend(int argc, char **argv)
>      Error *err = NULL;
>      int c, ret = 0;
>      char *options = NULL;
> -    QemuOptsList *create_opts = NULL;
> +    QemuOptsList *amend_opts = NULL;
>      QemuOpts *opts = NULL;
>      const char *fmt = NULL, *filename, *cache;
>      int flags;
> @@ -4031,11 +4031,11 @@ static int img_amend(int argc, char **argv)
>          goto out;
>      }
>  
> -    /* Every driver supporting amendment must have create_opts */
> -    assert(bs->drv->create_opts);
> +    /* Every driver supporting amendment must have amend_opts */
> +    assert(bs->drv->amend_opts);
>  
> -    create_opts = qemu_opts_append(create_opts, bs->drv->create_opts);
> -    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
> +    amend_opts = qemu_opts_append(amend_opts, bs->drv->amend_opts);
> +    opts = qemu_opts_create(amend_opts, NULL, 0, &error_abort);
>      qemu_opts_do_parse(opts, options, NULL, &err);
>      if (err) {
>          error_report_err(err);
> @@ -4058,7 +4058,7 @@ out:
>  out_no_progress:
>      blk_unref(blk);
>      qemu_opts_del(opts);
> -    qemu_opts_free(create_opts);
> +    qemu_opts_free(amend_opts);
>      g_free(options);
>  
>      if (ret) {
> -- 
> 2.17.2
> 

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 :|



  reply	other threads:[~2020-01-28 17:24 UTC|newest]

Thread overview: 84+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-14 19:33 [PATCH 00/13] LUKS: encryption slot management using amend interface Maxim Levitsky
2020-01-14 19:33 ` [PATCH 01/13] qcrypto: add generic infrastructure for crypto options amendment Maxim Levitsky
2020-01-28 16:59   ` Daniel P. Berrangé
2020-01-29 17:49     ` Maxim Levitsky
2020-01-14 19:33 ` [PATCH 02/13] qcrypto-luks: implement encryption key management Maxim Levitsky
2020-01-21  7:54   ` Markus Armbruster
2020-01-21 13:13     ` Maxim Levitsky
2020-01-28 17:11       ` Daniel P. Berrangé
2020-01-28 17:32         ` Daniel P. Berrangé
2020-01-29 17:54           ` Maxim Levitsky
2020-01-30 12:38           ` Kevin Wolf
2020-01-30 12:53             ` Daniel P. Berrangé
2020-01-30 14:23               ` Kevin Wolf
2020-01-30 14:30                 ` Daniel P. Berrangé
2020-01-30 14:53                 ` Markus Armbruster
2020-01-30 14:47               ` Markus Armbruster
2020-01-30 15:01                 ` Daniel P. Berrangé
2020-01-30 16:37                   ` Markus Armbruster
2020-02-05  8:24                     ` Markus Armbruster
2020-02-05  9:30                       ` Kevin Wolf
2020-02-05 10:03                         ` Markus Armbruster
2020-02-05 11:02                           ` Kevin Wolf
2020-02-05 14:31                             ` Markus Armbruster
2020-02-06 13:44                               ` Markus Armbruster
2020-02-06 13:49                                 ` Daniel P. Berrangé
2020-02-06 14:20                                   ` Max Reitz
2020-02-05 10:23                         ` Daniel P. Berrangé
2020-02-05 14:31                           ` Markus Armbruster
2020-02-06 13:20                             ` Markus Armbruster
2020-02-06 13:36                               ` Daniel P. Berrangé
2020-02-06 14:25                                 ` Kevin Wolf
2020-02-06 15:19                                   ` Markus Armbruster
2020-02-06 15:23                                     ` Maxim Levitsky
2020-01-30 15:45                 ` Maxim Levitsky
2020-01-28 17:21   ` Daniel P. Berrangé
2020-01-30 12:58     ` Maxim Levitsky
2020-02-15 14:51   ` QAPI schema for desired state of LUKS keyslots (was: [PATCH 02/13] qcrypto-luks: implement encryption key management) Markus Armbruster
2020-02-16  8:05     ` Maxim Levitsky
2020-02-17  6:45       ` QAPI schema for desired state of LUKS keyslots Markus Armbruster
2020-02-17  8:19         ` Maxim Levitsky
2020-02-17 10:37     ` QAPI schema for desired state of LUKS keyslots (was: [PATCH 02/13] qcrypto-luks: implement encryption key management) Kevin Wolf
2020-02-17 11:07       ` Maxim Levitsky
2020-02-24 14:46         ` Daniel P. Berrangé
2020-02-24 14:50           ` Maxim Levitsky
2020-02-17 12:28       ` QAPI schema for desired state of LUKS keyslots Markus Armbruster
2020-02-17 12:44         ` Eric Blake
2020-02-24 14:43         ` Daniel P. Berrangé
2020-02-24 14:45     ` QAPI schema for desired state of LUKS keyslots (was: [PATCH 02/13] qcrypto-luks: implement encryption key management) Daniel P. Berrangé
2020-02-25 12:15     ` Max Reitz
2020-02-25 16:48       ` QAPI schema for desired state of LUKS keyslots Markus Armbruster
2020-02-25 17:00         ` Max Reitz
2020-02-26  7:28           ` Markus Armbruster
2020-02-26  9:18             ` Maxim Levitsky
2020-02-25 17:18         ` Daniel P. Berrangé
2020-03-03  9:18     ` QAPI schema for desired state of LUKS keyslots (was: [PATCH 02/13] qcrypto-luks: implement encryption key management) Maxim Levitsky
2020-03-05 12:15       ` Maxim Levitsky
2020-01-14 19:33 ` [PATCH 03/13] block: amend: add 'force' option Maxim Levitsky
2020-01-14 19:33 ` [PATCH 04/13] block: amend: separate amend and create options for qemu-img Maxim Levitsky
2020-01-28 17:23   ` Daniel P. Berrangé [this message]
2020-01-30 15:54     ` Maxim Levitsky
2020-01-14 19:33 ` [PATCH 05/13] block/crypto: rename two functions Maxim Levitsky
2020-01-14 19:33 ` [PATCH 06/13] block/crypto: implement the encryption key management Maxim Levitsky
2020-01-28 17:27   ` Daniel P. Berrangé
2020-01-30 16:08     ` Maxim Levitsky
2020-01-14 19:33 ` [PATCH 07/13] qcow2: extend qemu-img amend interface with crypto options Maxim Levitsky
2020-01-28 17:30   ` Daniel P. Berrangé
2020-01-30 16:09     ` Maxim Levitsky
2020-01-14 19:33 ` [PATCH 08/13] iotests: filter few more luks specific create options Maxim Levitsky
2020-01-28 17:36   ` Daniel P. Berrangé
2020-01-30 16:12     ` Maxim Levitsky
2020-01-14 19:33 ` [PATCH 09/13] qemu-iotests: qemu-img tests for luks key management Maxim Levitsky
2020-01-14 19:33 ` [PATCH 10/13] block: add generic infrastructure for x-blockdev-amend qmp command Maxim Levitsky
2020-01-21  7:59   ` Markus Armbruster
2020-01-21 13:58     ` Maxim Levitsky
2020-01-14 19:33 ` [PATCH 11/13] block/crypto: implement blockdev-amend Maxim Levitsky
2020-01-28 17:40   ` Daniel P. Berrangé
2020-01-30 16:24     ` Maxim Levitsky
2020-01-14 19:33 ` [PATCH 12/13] block/qcow2: " Maxim Levitsky
2020-01-28 17:41   ` Daniel P. Berrangé
2020-01-14 19:33 ` [PATCH 13/13] iotests: add tests for blockdev-amend Maxim Levitsky
2020-01-14 21:16 ` [PATCH 00/13] LUKS: encryption slot management using amend interface no-reply
2020-01-16 14:01   ` Maxim Levitsky
2020-01-14 21:17 ` no-reply
2020-01-16 14:19   ` Maxim Levitsky

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200128172343.GW1446339@redhat.com \
    --to=berrange@redhat.com \
    --cc=armbru@redhat.com \
    --cc=jsnow@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=mlevitsk@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).