|
From: | Philippe Mathieu-Daudé |
Subject: | Re: [PATCH v3 4/4] migration/qapi: Drop @MigrationParameter enum |
Date: | Wed, 6 Sep 2023 12:14:54 +0200 |
User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.15.0 |
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
[Prev in Thread] | Current Thread | [Next in Thread] |