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

[Daniel P . Berrangé]

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.

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