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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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