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 <mlevi...@redhat.com> > > Reviewed-by: Daniel P. Berrangé <berra...@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 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 <mre...@redhat.com> Thanks! > > > + QemuOptsList *amend_opts; > > + > > Best regards, Maxim Levitsky