Re: [PATCH v6 04/14] block/amend: separate amend and create options for qemu-img

[Maxim Levitsky <mlevitsk@redhat.com>]

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 <mlevitsk@redhat.com>
> > Reviewed-by: Daniel P. Berrangé <berrange@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 <mreitz@redhat.com>
Thanks!
> 
> > +    QemuOptsList *amend_opts;
> > +
> 
> 

Best regards,
	Maxim Levitsky