[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6 04/14] block/amend: separate amend and create options for
|
From: |
Maxim Levitsky |
|
Subject: |
Re: [PATCH v6 04/14] block/amend: separate amend and create options for qemu-img |
|
Date: |
Sun, 17 May 2020 11:54:13 +0300 |
On Thu, 2020-05-14 at 14:28 +0200, Max Reitz wrote:
> On 10.05.20 15:40, 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.
> >
> > Signed-off-by: Maxim Levitsky <address@hidden>
> > Reviewed-by: Daniel P. Berrangé <address@hidden>
> > ---
> > 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 79fbad9d76..fc494c7591 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -5520,83 +5520,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" \
> > + } \
>
> I think the last line should have a comma in it (otherwise the final
> backslash doesn’t make much sense, because whenever we’d add a new
> option, we would need to modify the line anyway to insert a comma).
As discussed with Eric Blake, lets leave it as is.
>
> Speaking of adding option, this requires a rebase due to the
> compression_type option added (not trivial in the strict sense, but
> still straightforward to handle).
Done!
>
> > +
> > static QemuOptsList qcow2_create_opts = {
> > .name = "qcow2-create-opts",
> > .head = QTAILQ_HEAD_INITIALIZER(qcow2_create_opts.head),
> > .desc = {
> >
>
> [...]
>
> > + 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 */ }
>
> If QCOW_COMMON_OPTIONS were to already end in a comma (which I think it
> should), then it would become superfluous here.
>
> > }
> > };
>
> [...]
>
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index 952b2f033a..0a71357b50 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -412,6 +412,10 @@ struct BlockDriver {
> >
> > /* List of options for creating images, terminated by name == NULL */
> > QemuOptsList *create_opts;
> > +
> > + /* List of options for image amend*/
>
> I don’t suppose we have a coding style requirement for this, but I still
> think there should be a space before the closing asterisk.
Absolutely! Thanks!
>
> With those things fixed:
>
> Reviewed-by: Max Reitz <address@hidden>
Thanks!
>
> > + QemuOptsList *amend_opts;
> > +
>
>
Best regards,
Maxim Levitsky
- Re: [PATCH v6 02/14] qcrypto/luks: implement encryption key management, (continued)
- Re: [PATCH v6 04/14] block/amend: separate amend and create options for qemu-img,
Maxim Levitsky <=
[PATCH v6 05/14] block/amend: refactor qcow2 amend options, Maxim Levitsky, 2020/05/10
[PATCH v6 08/14] block/qcow2: extend qemu-img amend interface with crypto options, Maxim Levitsky, 2020/05/10
[PATCH v6 09/14] iotests: filter few more luks specific create options, Maxim Levitsky, 2020/05/10
[PATCH v6 10/14] iotests: qemu-img tests for luks key management, Maxim Levitsky, 2020/05/10