Re: [PATCH v3 4/4] migration/qapi: Drop @MigrationParameter enum

[Philippe Mathieu-Daudé]

On 6/9/23 11:00, Daniel P. Berrangé wrote:
> On Wed, Sep 06, 2023 at 06:42:16AM +0200, Philippe Mathieu-Daudé wrote:
> > On 5/9/23 18:23, Peter Xu wrote:
> > > Drop the enum in qapi because it is never used in QMP APIs.  Instead making
> > > it an internal definition for QEMU so that we can decouple it from QAPI,
> > > and also we can deduplicate the QAPI documentations.
> > >
> > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >    qapi/migration.json            | 179 ---------------------------------
> > >    migration/options.h            |  47 +++++++++
> > >    migration/migration-hmp-cmds.c |   3 +-
> > >    migration/options.c            |  51 ++++++++++
> > >    4 files changed, 100 insertions(+), 180 deletions(-)
> >
> >
> > > diff --git a/migration/options.h b/migration/options.h
> > > index 124a5d450f..4591545c62 100644
> > > --- a/migration/options.h
> > > +++ b/migration/options.h
> > > @@ -66,6 +66,53 @@ bool migrate_cap_set(int cap, bool value, Error **errp);
> > >    /* parameters */
> > > +typedef enum {
> > > +    MIGRATION_PARAMETER_ANNOUNCE_INITIAL,
> > > +    MIGRATION_PARAMETER_ANNOUNCE_MAX,
> > > +    MIGRATION_PARAMETER_ANNOUNCE_ROUNDS,
> > > +    MIGRATION_PARAMETER_ANNOUNCE_STEP,
> > > +    MIGRATION_PARAMETER_COMPRESS_LEVEL,
> > > +    MIGRATION_PARAMETER_COMPRESS_THREADS,
> > > +    MIGRATION_PARAMETER_DECOMPRESS_THREADS,
> > > +    MIGRATION_PARAMETER_COMPRESS_WAIT_THREAD,
> > > +    MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD,
> > > +    MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL,
> > > +    MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT,
> > > +    MIGRATION_PARAMETER_CPU_THROTTLE_TAILSLOW,
> > > +    MIGRATION_PARAMETER_TLS_CREDS,
> > > +    MIGRATION_PARAMETER_TLS_HOSTNAME,
> > > +    MIGRATION_PARAMETER_TLS_AUTHZ,
> > > +    MIGRATION_PARAMETER_MAX_BANDWIDTH,
> > > +    MIGRATION_PARAMETER_DOWNTIME_LIMIT,
> > > +    MIGRATION_PARAMETER_X_CHECKPOINT_DELAY,
> > > +    MIGRATION_PARAMETER_BLOCK_INCREMENTAL,
> > > +    MIGRATION_PARAMETER_MULTIFD_CHANNELS,
> > > +    MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE,
> > > +    MIGRATION_PARAMETER_MAX_POSTCOPY_BANDWIDTH,
> > > +    MIGRATION_PARAMETER_MAX_CPU_THROTTLE,
> > > +    MIGRATION_PARAMETER_MULTIFD_COMPRESSION,
> > > +    MIGRATION_PARAMETER_MULTIFD_ZLIB_LEVEL,
> > > +    MIGRATION_PARAMETER_MULTIFD_ZSTD_LEVEL,
> > > +    MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING,
> > > +    MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD,
> > > +    MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT,
> > > +    MIGRATION_PARAMETER__MAX,
> >
> > MIGRATION_PARAMETER__MAX is not part of the enum, so:
> >
> >     #define MIGRATION_PARAMETER__MAX \
> >         (MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT + 1)
>
> IMHO the way it currently is written is better, because the
> __MAX value is guaranteed to always have the right max value
> without needing to be manually changed when new params are
> added. Note this matches the code style used by the QAPI
> enum generator too.

This concern comes from a previous discussion with Richard (which I
can't find now in the archives) where he explained to me __MAX is not
part of the enum set, thus reduces the coverage of compiler sanitizers
/ optimizers, and could introduce subtle bugs.

This motivated this series:
https://lore.kernel.org/qemu-devel/20230315112811.22355-4-philmd@linaro.org/
which should have changed that generated QAPI enum.

(I didn't respin that series because I couldn't find an easy way to
 handle conditionals, see
 https://lore.kernel.org/qemu-devel/87sfdyaq0m.fsf@pond.sub.org/)

Back to this patch, I don't object to having MIGRATION_PARAMETER__MAX
in the enum, but I'd rather have the suggestion below considered.

Thanks,

Phil.

> > > +} MigrationParameter;
> > > +
> > > +extern const char *MigrationParameter_string[MIGRATION_PARAMETER__MAX];
> > > +#define  MigrationParameter_str(p)  MigrationParameter_string[p]
> >
> > Hmm this is only used once by HMP. Following our style I suggest here:
> >
> >   const char *const MigrationParameter_string(enum MigrationParameter param);
> >
> > And in options.c:
> >
> >   static const char *const MigrationParameter_str[MIGRATION_PARAMETER__MAX] =
> > {
> >      ...
> >   };
> >
> >   const char *const MigrationParameter_string(enum MigrationParameter param)
> >   {
> >       return MigrationParameter_str[param];
> >   }
> >
> > > +
> > > +/**
> > > + * @MigrationParameter_from_str(): Parse string into a MigrationParameter
> > > + *
> > > + * @param: input string
> > > + * @errp: error message if failed to parse the string
> > > + *
> > > + * Returns MigrationParameter enum (>=0) if succeed, or negative otherwise
> > > + * which will always setup @errp.
> > > + */
> > > +int MigrationParameter_from_str(const char *param, Error **errp);
> > > +
> > >    const BitmapMigrationNodeAliasList *migrate_block_bitmap_mapping(void);
> > >    bool migrate_has_block_bitmap_mapping(void);
> >
> > With the changes:
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> With regards,
> Daniel