| Message ID | 20230905162335.235619-5-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi/migration: Dedup migration parameter objects and fix tls-authz crash | expand |
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) > +} 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>
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
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
On Wed, Sep 06, 2023 at 12:14:54PM +0200, Philippe Mathieu-Daudé wrote: > 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/) Oh, I completely forgot about that series. So the original problem is that with '-Wswitch' present, if the switched variable is an enum type, the compiler complains if you don't list all possible enum values, or have a default: clause. Thus the existance of __MAX forces use to add case ...__MAX, or have a default, and you wanted to eliminate that requirement. Or the surface that sounds reasonable, but I actually think that is the conceptually wrong approach from a robustness POV. C (and some other languages) are terrible wrt enum declared constants vs actual stored values. You can have a variable declared KeyValueKind and it can store absolutely any integer value at all, whether intentionally, or by a code mistake or by data corruption. In your example you modified: switch (key->key->type) { case KEY_VALUE_KIND_NUMBER: qcode = qemu_input_key_number_to_qcode(key->key->u.number.data); name = QKeyCode_str(qcode); trace_input_event_key_number(idx, key->key->u.number.data, name, key->down); break; case KEY_VALUE_KIND_QCODE: name = QKeyCode_str(key->key->u.qcode.data); trace_input_event_key_qcode(idx, name, key->down); break; case KEY_VALUE_KIND__MAX: /* keep gcc happy */ break; } to remove KEY_VALUE_KIND__MAX. What we should actually do IMHO is to either change it to default: g_assert_not_reached(); Or get extra paranoid and -Wswitch-enum too and list both together case KEY_VALUE_KIND__MAX: default: g_assert_not_reached(); This forces us to validate every enum case, and also protect against out of range values. This is a little more verbose to code, but I can't say it has been a maint problem in libvirt where we've followed this approach with -Wswitch-enum and _MAX constants. > 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. I just prefer to see consistency in approach across the codebase, and currently we use __MAX approach. With regards, Daniel
On Wed, Sep 06, 2023 at 11:46:16AM +0100, Daniel P. Berrangé wrote: > On Wed, Sep 06, 2023 at 12:14:54PM +0200, Philippe Mathieu-Daudé wrote: > > 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/) > > Oh, I completely forgot about that series. > > So the original problem is that with '-Wswitch' present, if the > switched variable is an enum type, the compiler complains if you > don't list all possible enum values, or have a default: clause. > > Thus the existance of __MAX forces use to add case ...__MAX, or > have a default, and you wanted to eliminate that requirement. > > Or the surface that sounds reasonable, but I actually think that > is the conceptually wrong approach from a robustness POV. > > C (and some other languages) are terrible wrt enum declared > constants vs actual stored values. > > You can have a variable declared KeyValueKind and it can store > absolutely any integer value at all, whether intentionally, > or by a code mistake or by data corruption. > > In your example you modified: > > switch (key->key->type) { > case KEY_VALUE_KIND_NUMBER: > qcode = qemu_input_key_number_to_qcode(key->key->u.number.data); > name = QKeyCode_str(qcode); > trace_input_event_key_number(idx, key->key->u.number.data, > name, key->down); > break; > case KEY_VALUE_KIND_QCODE: > name = QKeyCode_str(key->key->u.qcode.data); > trace_input_event_key_qcode(idx, name, key->down); > break; > case KEY_VALUE_KIND__MAX: > /* keep gcc happy */ > break; > } > > to remove KEY_VALUE_KIND__MAX. > > What we should actually do IMHO is to either change it to > > default: > g_assert_not_reached(); > > Or get extra paranoid and -Wswitch-enum too and list both > together > > case KEY_VALUE_KIND__MAX: > default: > g_assert_not_reached(); > > This forces us to validate every enum case, and also protect > against out of range values. > > This is a little more verbose to code, but I can't say it > has been a maint problem in libvirt where we've followed > this approach with -Wswitch-enum and _MAX constants. > > > 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. > > I just prefer to see consistency in approach across the codebase, and > currently we use __MAX approach. Personally I prefer the same. I'll squash below change into the patch (with Phil's R-b): =============== diff --git a/migration/options.h b/migration/options.h index 4591545c62..2e4fa17351 100644 --- a/migration/options.h +++ b/migration/options.h @@ -99,8 +99,7 @@ typedef enum { MIGRATION_PARAMETER__MAX, } MigrationParameter; -extern const char *MigrationParameter_string[MIGRATION_PARAMETER__MAX]; -#define MigrationParameter_str(p) MigrationParameter_string[p] +const char *MigrationParameter_str(MigrationParameter p); /** * @MigrationParameter_from_str(): Parse string into a MigrationParameter diff --git a/migration/options.c b/migration/options.c index c9b90d932d..e87c9667d1 100644 --- a/migration/options.c +++ b/migration/options.c @@ -84,7 +84,7 @@ #define DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT_PERIOD 1000 /* milliseconds */ #define DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT 1 /* MB/s */ -const char *MigrationParameter_string[MIGRATION_PARAMETER__MAX] = { +static const char *const MigrationParameter_string[MIGRATION_PARAMETER__MAX] = { [MIGRATION_PARAMETER_ANNOUNCE_INITIAL] = "announce-initial", [MIGRATION_PARAMETER_ANNOUNCE_MAX] = "announce-max", [MIGRATION_PARAMETER_ANNOUNCE_ROUNDS] = "announce-rounds", @@ -116,6 +116,11 @@ const char *MigrationParameter_string[MIGRATION_PARAMETER__MAX] = { [MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT] = "vcpu-dirty-limit", }; +const char *MigrationParameter_str(MigrationParameter p) +{ + return MigrationParameter_string[p]; +} + int MigrationParameter_from_str(const char *param, Error **errp) { int i; =============== Thanks,
Peter Xu <peterx@redhat.com> writes: > 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> The enum is defined in the QAPI schema, because that gives us easy conversion to and from strings (it also gives us visit_type_MigrationParameter(), but we don't care for that). I assume you move it out of the QAPI schema just so you don't have to maintain the useless doc comment anymore. Correct? Let me take a step back. Reasons for defining a type in the QAPI schema: 0. QMP commands or events use the type. 1. CLI uses the type (QAPIfied option arguments). 2. QOM properties use the type. 3. Use internal to QEMU, say for easy conversion to and from strings. Generated documentation includes all the types (unlike query-qmp-schema, which describes exactly QMP). Issues: 1. Documentation fails to connect types to the QOM properties using them. Just one part of the bigger QOM documentation misery. 2. Likewise, types are not connected to the CLI options using them. 3. Types used only internally appear in documentation. > --- > 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(-) At a glance, the diffstat looks like a win. Less so once we drill into it. 160 of 180 lines deleted from qapi/migration.json are doc comment. The other 20 lines deleted there you replace by 100 lines of C code. All this just to get rid of the useless doc comment. Hmm. We can't just delete it because the QAPI generator insists on documentation. Loophole... Here's the stupidest solution that could possibly work: ## # @MigrationParameter: # # TODO: elide from generated documentation (type is used only # internally, and not visible in QMP) # # Features: # # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period # are experimental. # # Since: 2.4 ## Works because the QAPI generator currently doesn't flag missing member documentation, and quietly substitutes "Not documented" instead. Looks like "MigrationParameter" (Enum) --------------------------- Values ~~~~~~ "announce-initial" Not documented "announce-max" Not documented and so forth. Sure ugly, but is it really worse than before? It's now obviously useless, whereas before it was unobviously useless. This will break when we tighten up the QAPI generator to require member documentation. Along we a few hundred other violators. We might want to add a way to say "members intentionally undocumented". Could be useful for qapi/ui.json's QKeyCode. Most of its 162 members don't really need documentation... > diff --git a/qapi/migration.json b/qapi/migration.json > index 45d69787ae..eeb1878c4f 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -672,185 +672,6 @@ > 'bitmaps': [ 'BitmapMigrationBitmapAlias' ] > } } > > -## > -# @MigrationParameter: > -# > -# Migration parameters enumeration > -# > -# @announce-initial: Initial delay (in milliseconds) before sending > -# the first announce (Since 4.0) > -# > -# @announce-max: Maximum delay (in milliseconds) between packets in > -# the announcement (Since 4.0) > -# > -# @announce-rounds: Number of self-announce packets sent after > -# migration (Since 4.0) > -# > -# @announce-step: Increase in delay (in milliseconds) between > -# subsequent packets in the announcement (Since 4.0) > -# > -# @compress-level: Set the compression level to be used in live > -# migration, the compression level is an integer between 0 and 9, > -# where 0 means no compression, 1 means the best compression > -# speed, and 9 means best compression ratio which will consume > -# more CPU. > -# > -# @compress-threads: Set compression thread count to be used in live > -# migration, the compression thread count is an integer between 1 > -# and 255. > -# > -# @compress-wait-thread: Controls behavior when all compression > -# threads are currently busy. If true (default), wait for a free > -# compression thread to become available; otherwise, send the page > -# uncompressed. (Since 3.1) > -# > -# @decompress-threads: Set decompression thread count to be used in > -# live migration, the decompression thread count is an integer > -# between 1 and 255. Usually, decompression is at least 4 times as > -# fast as compression, so set the decompress-threads to the number > -# about 1/4 of compress-threads is adequate. > -# > -# @throttle-trigger-threshold: The ratio of bytes_dirty_period and > -# bytes_xfer_period to trigger throttling. It is expressed as > -# percentage. The default value is 50. (Since 5.0) > -# > -# @cpu-throttle-initial: Initial percentage of time guest cpus are > -# throttled when migration auto-converge is activated. The > -# default value is 20. (Since 2.7) > -# > -# @cpu-throttle-increment: throttle percentage increase each time > -# auto-converge detects that migration is not making progress. > -# The default value is 10. (Since 2.7) > -# > -# @cpu-throttle-tailslow: Make CPU throttling slower at tail stage At > -# the tail stage of throttling, the Guest is very sensitive to CPU > -# percentage while the @cpu-throttle -increment is excessive > -# usually at tail stage. If this parameter is true, we will > -# compute the ideal CPU percentage used by the Guest, which may > -# exactly make the dirty rate match the dirty rate threshold. > -# Then we will choose a smaller throttle increment between the one > -# specified by @cpu-throttle-increment and the one generated by > -# ideal CPU percentage. Therefore, it is compatible to > -# traditional throttling, meanwhile the throttle increment won't > -# be excessive at tail stage. The default value is false. (Since > -# 5.1) > -# > -# @tls-creds: ID of the 'tls-creds' object that provides credentials > -# for establishing a TLS connection over the migration data > -# channel. On the outgoing side of the migration, the credentials > -# must be for a 'client' endpoint, while for the incoming side the > -# credentials must be for a 'server' endpoint. Setting this will > -# enable TLS for all migrations. The default is unset, resulting > -# in unsecured migration at the QEMU level. (Since 2.7) > -# > -# @tls-hostname: hostname of the target host for the migration. This > -# is required when using x509 based TLS credentials and the > -# migration URI does not already include a hostname. For example > -# if using fd: or exec: based migration, the hostname must be > -# provided so that the server's x509 certificate identity can be > -# validated. (Since 2.7) > -# > -# @tls-authz: ID of the 'authz' object subclass that provides access > -# control checking of the TLS x509 certificate distinguished name. > -# This object is only resolved at time of use, so can be deleted > -# and recreated on the fly while the migration server is active. > -# If missing, it will default to denying access (Since 4.0) > -# > -# @max-bandwidth: to set maximum speed for migration. maximum speed > -# in bytes per second. (Since 2.8) > -# > -# @downtime-limit: set maximum tolerated downtime for migration. > -# maximum downtime in milliseconds (Since 2.8) > -# > -# @x-checkpoint-delay: The delay time (in ms) between two COLO > -# checkpoints in periodic mode. (Since 2.8) > -# > -# @block-incremental: Affects how much storage is migrated when the > -# block migration capability is enabled. When false, the entire > -# storage backing chain is migrated into a flattened image at the > -# destination; when true, only the active qcow2 layer is migrated > -# and the destination must already have access to the same backing > -# chain as was used on the source. (since 2.10) > -# > -# @multifd-channels: Number of channels used to migrate data in > -# parallel. This is the same number that the number of sockets > -# used for migration. The default value is 2 (since 4.0) > -# > -# @xbzrle-cache-size: cache size to be used by XBZRLE migration. It > -# needs to be a multiple of the target page size and a power of 2 > -# (Since 2.11) > -# > -# @max-postcopy-bandwidth: Background transfer bandwidth during > -# postcopy. Defaults to 0 (unlimited). In bytes per second. > -# (Since 3.0) > -# > -# @max-cpu-throttle: maximum cpu throttle percentage. Defaults to 99. > -# (Since 3.1) > -# > -# @multifd-compression: Which compression method to use. Defaults to > -# none. (Since 5.0) > -# > -# @multifd-zlib-level: Set the compression level to be used in live > -# migration, the compression level is an integer between 0 and 9, > -# where 0 means no compression, 1 means the best compression > -# speed, and 9 means best compression ratio which will consume > -# more CPU. Defaults to 1. (Since 5.0) > -# > -# @multifd-zstd-level: Set the compression level to be used in live > -# migration, the compression level is an integer between 0 and 20, > -# where 0 means no compression, 1 means the best compression > -# speed, and 20 means best compression ratio which will consume > -# more CPU. Defaults to 1. (Since 5.0) > -# > -# @block-bitmap-mapping: Maps block nodes and bitmaps on them to > -# aliases for the purpose of dirty bitmap migration. Such aliases > -# may for example be the corresponding names on the opposite site. > -# The mapping must be one-to-one, but not necessarily complete: On > -# the source, unmapped bitmaps and all bitmaps on unmapped nodes > -# will be ignored. On the destination, encountering an unmapped > -# alias in the incoming migration stream will result in a report, > -# and all further bitmap migration data will then be discarded. > -# Note that the destination does not know about bitmaps it does > -# not receive, so there is no limitation or requirement regarding > -# the number of bitmaps received, or how they are named, or on > -# which nodes they are placed. By default (when this parameter > -# has never been set), bitmap names are mapped to themselves. > -# Nodes are mapped to their block device name if there is one, and > -# to their node name otherwise. (Since 5.2) > -# > -# @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty > -# limit during live migration. Should be in the range 1 to 1000ms. > -# Defaults to 1000ms. (Since 8.1) > -# > -# @vcpu-dirty-limit: Dirtyrate limit (MB/s) during live migration. > -# Defaults to 1. (Since 8.1) > -# > -# Features: > -# > -# @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period > -# are experimental. > -# > -# Since: 2.4 > -## > -{ 'enum': 'MigrationParameter', > - 'data': ['announce-initial', 'announce-max', > - 'announce-rounds', 'announce-step', > - 'compress-level', 'compress-threads', 'decompress-threads', > - 'compress-wait-thread', 'throttle-trigger-threshold', > - 'cpu-throttle-initial', 'cpu-throttle-increment', > - 'cpu-throttle-tailslow', > - 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth', > - 'downtime-limit', > - { 'name': 'x-checkpoint-delay', 'features': [ 'unstable' ] }, > - 'block-incremental', > - 'multifd-channels', > - 'xbzrle-cache-size', 'max-postcopy-bandwidth', > - 'max-cpu-throttle', 'multifd-compression', > - 'multifd-zlib-level', 'multifd-zstd-level', > - 'block-bitmap-mapping', > - { 'name': 'x-vcpu-dirty-limit-period', 'features': ['unstable'] }, > - 'vcpu-dirty-limit'] } > - > ## > # @migrate-set-parameters: > # > 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, > +} MigrationParameter; > + > +extern const char *MigrationParameter_string[MIGRATION_PARAMETER__MAX]; > +#define MigrationParameter_str(p) MigrationParameter_string[p] This is effectifely the code we no longer generate into qapi-types-migration.h: typedef enum MigrationParameter { 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, } MigrationParameter; #define MigrationParameter_str(val) \ qapi_enum_lookup(&MigrationParameter_lookup, (val)) extern const QEnumLookup MigrationParameter_lookup; > + > +/** > + * @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); > + This is in addition. More below next to its definition. > const BitmapMigrationNodeAliasList *migrate_block_bitmap_mapping(void); > bool migrate_has_block_bitmap_mapping(void); > > diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c > index 88a8ccb475..0a35a87b7e 100644 > --- a/migration/migration-hmp-cmds.c > +++ b/migration/migration-hmp-cmds.c > @@ -31,6 +31,7 @@ > #include "ui/qemu-spice.h" > #include "sysemu/sysemu.h" > #include "migration.h" > +#include "migration/options.h" > > static void migration_global_dump(Monitor *mon) > { > @@ -505,7 +506,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) > Error *err = NULL; > int val, ret; > > - val = qapi_enum_parse(&MigrationParameter_lookup, param, -1, &err); > + val = MigrationParameter_from_str(param, &err); > if (val < 0) { > goto cleanup; > } You replace qapi_enum_parse() by MigrationParameter_from_str(). > diff --git a/migration/options.c b/migration/options.c > index 12e392f68c..c9b90d932d 100644 > --- a/migration/options.c > +++ b/migration/options.c > @@ -84,6 +84,57 @@ > #define DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT_PERIOD 1000 /* milliseconds */ > #define DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT 1 /* MB/s */ > > +const char *MigrationParameter_string[MIGRATION_PARAMETER__MAX] = { > + [MIGRATION_PARAMETER_ANNOUNCE_INITIAL] = "announce-initial", > + [MIGRATION_PARAMETER_ANNOUNCE_MAX] = "announce-max", > + [MIGRATION_PARAMETER_ANNOUNCE_ROUNDS] = "announce-rounds", > + [MIGRATION_PARAMETER_ANNOUNCE_STEP] = "announce-step", > + [MIGRATION_PARAMETER_COMPRESS_LEVEL] = "compress-level", > + [MIGRATION_PARAMETER_COMPRESS_THREADS] = "compress-threads", > + [MIGRATION_PARAMETER_DECOMPRESS_THREADS] = "decompress-threads", > + [MIGRATION_PARAMETER_COMPRESS_WAIT_THREAD] = "compress-wait-thread", > + [MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD] = "throttle-trigger-threshold", > + [MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL] = "cpu-throttle-initial", > + [MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT] = "cpu-throttle-increment", > + [MIGRATION_PARAMETER_CPU_THROTTLE_TAILSLOW] = "cpu-throttle-tailslow", > + [MIGRATION_PARAMETER_TLS_CREDS] = "tls-creds", > + [MIGRATION_PARAMETER_TLS_HOSTNAME] = "tls-hostname", > + [MIGRATION_PARAMETER_TLS_AUTHZ] = "tls-authz", > + [MIGRATION_PARAMETER_MAX_BANDWIDTH] = "max-bandwidth", > + [MIGRATION_PARAMETER_DOWNTIME_LIMIT] = "downtime-limit", > + [MIGRATION_PARAMETER_X_CHECKPOINT_DELAY] = "x-checkpoint-delay", > + [MIGRATION_PARAMETER_BLOCK_INCREMENTAL] = "block-incremental", > + [MIGRATION_PARAMETER_MULTIFD_CHANNELS] = "multifd-channels", > + [MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE] = "xbzrle-cache-size", > + [MIGRATION_PARAMETER_MAX_POSTCOPY_BANDWIDTH] = "max-postcopy-bandwidth", > + [MIGRATION_PARAMETER_MAX_CPU_THROTTLE] = "max-cpu-throttle", > + [MIGRATION_PARAMETER_MULTIFD_COMPRESSION] = "multifd-compression", > + [MIGRATION_PARAMETER_MULTIFD_ZLIB_LEVEL] = "multifd-zlib-level", > + [MIGRATION_PARAMETER_MULTIFD_ZSTD_LEVEL] = "multifd-zstd-level", > + [MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING] = "block-bitmap-mapping", > + [MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD] = "x-vcpu-dirty-limit-period", > + [MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT] = "vcpu-dirty-limit", > +}; This is almost the code we no longer generate into qapi-types-migration.c: const QEnumLookup MigrationParameter_lookup = { .array = (const char *const[]) { [MIGRATION_PARAMETER_ANNOUNCE_INITIAL] = "announce-initial", [MIGRATION_PARAMETER_ANNOUNCE_MAX] = "announce-max", [MIGRATION_PARAMETER_ANNOUNCE_ROUNDS] = "announce-rounds", [MIGRATION_PARAMETER_ANNOUNCE_STEP] = "announce-step", [MIGRATION_PARAMETER_COMPRESS_LEVEL] = "compress-level", [MIGRATION_PARAMETER_COMPRESS_THREADS] = "compress-threads", [MIGRATION_PARAMETER_DECOMPRESS_THREADS] = "decompress-threads", [MIGRATION_PARAMETER_COMPRESS_WAIT_THREAD] = "compress-wait-thread", [MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD] = "throttle-trigger-threshold", [MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL] = "cpu-throttle-initial", [MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT] = "cpu-throttle-increment", [MIGRATION_PARAMETER_CPU_THROTTLE_TAILSLOW] = "cpu-throttle-tailslow", [MIGRATION_PARAMETER_TLS_CREDS] = "tls-creds", [MIGRATION_PARAMETER_TLS_HOSTNAME] = "tls-hostname", [MIGRATION_PARAMETER_TLS_AUTHZ] = "tls-authz", [MIGRATION_PARAMETER_MAX_BANDWIDTH] = "max-bandwidth", [MIGRATION_PARAMETER_DOWNTIME_LIMIT] = "downtime-limit", [MIGRATION_PARAMETER_X_CHECKPOINT_DELAY] = "x-checkpoint-delay", [MIGRATION_PARAMETER_BLOCK_INCREMENTAL] = "block-incremental", [MIGRATION_PARAMETER_MULTIFD_CHANNELS] = "multifd-channels", [MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE] = "xbzrle-cache-size", [MIGRATION_PARAMETER_MAX_POSTCOPY_BANDWIDTH] = "max-postcopy-bandwidth", [MIGRATION_PARAMETER_MAX_CPU_THROTTLE] = "max-cpu-throttle", [MIGRATION_PARAMETER_MULTIFD_COMPRESSION] = "multifd-compression", [MIGRATION_PARAMETER_MULTIFD_ZLIB_LEVEL] = "multifd-zlib-level", [MIGRATION_PARAMETER_MULTIFD_ZSTD_LEVEL] = "multifd-zstd-level", [MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING] = "block-bitmap-mapping", [MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD] = "x-vcpu-dirty-limit-period", [MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT] = "vcpu-dirty-limit", }, .special_features = (const unsigned char[MIGRATION_PARAMETER__MAX]) { [MIGRATION_PARAMETER_X_CHECKPOINT_DELAY] = 1u << QAPI_UNSTABLE, [MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD] = 1u << QAPI_UNSTABLE, }, .size = MIGRATION_PARAMETER__MAX }; The difference is .special_features[]. Used only by visitor code, which you don't need. Okay. > + > +int MigrationParameter_from_str(const char *param, Error **errp) > +{ > + int i; > + > + if (!param) { > + error_setg(errp, "Missing parameter value"); > + return -1; > + } > + > + for (i = 0; i < MIGRATION_PARAMETER__MAX; i++) { > + if (!strcmp(param, MigrationParameter_string[i])) { > + return i; > + } > + } > + > + error_setg(errp, "Invalid parameter value: %s", param); > + return -1; > +} > + This is qapi_enum_parse() specialized for MigrationParameter_string[]. Not needed if we simply keep using qapi_enum_parse(). > Property migration_properties[] = { > DEFINE_PROP_BOOL("store-global-state", MigrationState, > store_global_state, true),
On Tue, Sep 26, 2023 at 10:43:22PM +0200, Markus Armbruster wrote: > Loophole... Here's the stupidest solution that could possibly work: > > ## > # @MigrationParameter: > # > # TODO: elide from generated documentation (type is used only > # internally, and not visible in QMP) > # > # Features: > # > # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period > # are experimental. > # > # Since: 2.4 > ## > > Works because the QAPI generator currently doesn't flag missing member > documentation, and quietly substitutes "Not documented" instead. Didn't work for me.. In file included from ../qapi/qapi-schema.json:61: ../qapi/migration.json:681:1: unexpected de-indent (expected at least 4 spaces) L681 points to the "Features:". But maybe I did it wrong somewhere? > Looks like > > "MigrationParameter" (Enum) > --------------------------- > > > Values > ~~~~~~ > > "announce-initial" > Not documented > > "announce-max" > Not documented > > and so forth. Sure ugly, but is it really worse than before? It's now > obviously useless, whereas before it was unobviously useless. > > This will break when we tighten up the QAPI generator to require member > documentation. Along we a few hundred other violators. > > We might want to add a way to say "members intentionally undocumented". > Could be useful for qapi/ui.json's QKeyCode. Most of its 162 members > don't really need documentation... Yes I'd be super happy if we can declare that in qapi/. Please let me know if I missed something above on the trick; you're right it's only about the documentation we want to get rid of. If we can achieve that with qapi generating the helpers that'll definitely be perfect. Thanks,
Peter Xu <peterx@redhat.com> writes: > On Tue, Sep 26, 2023 at 10:43:22PM +0200, Markus Armbruster wrote: >> Loophole... Here's the stupidest solution that could possibly work: >> >> ## >> # @MigrationParameter: >> # >> # TODO: elide from generated documentation (type is used only >> # internally, and not visible in QMP) >> # >> # Features: >> # >> # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period >> # are experimental. >> # >> # Since: 2.4 >> ## >> >> Works because the QAPI generator currently doesn't flag missing member >> documentation, and quietly substitutes "Not documented" instead. > > Didn't work for me.. > > In file included from ../qapi/qapi-schema.json:61: > ../qapi/migration.json:681:1: unexpected de-indent (expected at least 4 spaces) > > L681 points to the "Features:". > > But maybe I did it wrong somewhere? Parser bug? Moving the TODO down some works around the issue: ## # @MigrationParameter: # # Features: # # @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period # are experimental. # # TODO: elide from generated documentation (type is used only # internally, and not visible in QMP) # # Since: 2.4 ## Weird. Better, because even stupider: drop the feature flags. They have no effect on internal use, and there is no external use. ## # @MigrationParameter: # # TODO: elide from generated documentation (type is used only # internally, and not visible in QMP) # # Since: 2.4 ## { 'enum': 'MigrationParameter', 'data': ['announce-initial', 'announce-max', 'announce-rounds', 'announce-step', 'compress-level', 'compress-threads', 'decompress-threads', 'compress-wait-thread', 'throttle-trigger-threshold', 'cpu-throttle-initial', 'cpu-throttle-increment', 'cpu-throttle-tailslow', 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth', 'downtime-limit', 'x-checkpoint-delay', 'block-incremental', 'multifd-channels', 'xbzrle-cache-size', 'max-postcopy-bandwidth', 'max-cpu-throttle', 'multifd-compression', 'multifd-zlib-level', 'multifd-zstd-level', 'block-bitmap-mapping', 'x-vcpu-dirty-limit-period', 'vcpu-dirty-limit'] } >> Looks like >> >> "MigrationParameter" (Enum) >> --------------------------- >> >> >> Values >> ~~~~~~ >> >> "announce-initial" >> Not documented >> >> "announce-max" >> Not documented >> >> and so forth. Sure ugly, but is it really worse than before? It's now >> obviously useless, whereas before it was unobviously useless. >> >> This will break when we tighten up the QAPI generator to require member >> documentation. Along we a few hundred other violators. >> >> We might want to add a way to say "members intentionally undocumented". >> Could be useful for qapi/ui.json's QKeyCode. Most of its 162 members >> don't really need documentation... > > Yes I'd be super happy if we can declare that in qapi/. > > Please let me know if I missed something above on the trick; you're right > it's only about the documentation we want to get rid of. If we can achieve > that with qapi generating the helpers that'll definitely be perfect. Done, I think :)
On Mon, Oct 16, 2023 at 08:29:58AM +0200, Markus Armbruster wrote: > Better, because even stupider: drop the feature flags. They have no > effect on internal use, and there is no external use. > > ## > # @MigrationParameter: > # > # TODO: elide from generated documentation (type is used only > # internally, and not visible in QMP) > # > # Since: 2.4 > ## > { 'enum': 'MigrationParameter', > 'data': ['announce-initial', 'announce-max', > 'announce-rounds', 'announce-step', > 'compress-level', 'compress-threads', 'decompress-threads', > 'compress-wait-thread', 'throttle-trigger-threshold', > 'cpu-throttle-initial', 'cpu-throttle-increment', > 'cpu-throttle-tailslow', > 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth', > 'downtime-limit', > 'x-checkpoint-delay', > 'block-incremental', > 'multifd-channels', > 'xbzrle-cache-size', 'max-postcopy-bandwidth', > 'max-cpu-throttle', 'multifd-compression', > 'multifd-zlib-level', 'multifd-zstd-level', > 'block-bitmap-mapping', > 'x-vcpu-dirty-limit-period', > 'vcpu-dirty-limit'] } Didn't work either, unfortunately.. Compile is fine, but I still see the lines generated in qemu-qmp-ref.7. MigrationParameter (Enum) Values announce-initial Not documented announce-max Not documented announce-rounds Not documented announce-step Not documented compress-level Not documented [...] Patch attached. Thanks, ===8<=== From 6e9355f2b5e1ad21de1e13114055390077f34ca0 Mon Sep 17 00:00:00 2001 From: Peter Xu <peterx@redhat.com> Date: Mon, 16 Oct 2023 12:00:44 -0400 Subject: [PATCH] migration/qapi: Drop documentation for MigrationParameter @MigrationParameter shouldn't be exposed in QAPI schema. It should only be used in QEMU internally. Documentation is not required for this object. Keeping it under qapi/ still makes sense, so as to benefit QEMU from auto-generated helpers around the object to reduce hand written codes. After all, the documentation duplicates with @MigrationSetParameters and @MigrationParameters which are real exported QAPI objects. It's more challenging to deduplicate all three into one, but drop the easy one first by leveraging a loophole in QAPI generator. When at it, redo the newlines so one parameter per line. Suggested-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> --- qapi/migration.json | 203 ++++++-------------------------------------- 1 file changed, 27 insertions(+), 176 deletions(-) diff --git a/qapi/migration.json b/qapi/migration.json index db3df12d6c..7b9054433a 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -673,190 +673,41 @@ ## # @MigrationParameter: # -# Migration parameters enumeration -# -# @announce-initial: Initial delay (in milliseconds) before sending -# the first announce (Since 4.0) -# -# @announce-max: Maximum delay (in milliseconds) between packets in -# the announcement (Since 4.0) -# -# @announce-rounds: Number of self-announce packets sent after -# migration (Since 4.0) -# -# @announce-step: Increase in delay (in milliseconds) between -# subsequent packets in the announcement (Since 4.0) -# -# @compress-level: Set the compression level to be used in live -# migration, the compression level is an integer between 0 and 9, -# where 0 means no compression, 1 means the best compression -# speed, and 9 means best compression ratio which will consume -# more CPU. -# -# @compress-threads: Set compression thread count to be used in live -# migration, the compression thread count is an integer between 1 -# and 255. -# -# @compress-wait-thread: Controls behavior when all compression -# threads are currently busy. If true (default), wait for a free -# compression thread to become available; otherwise, send the page -# uncompressed. (Since 3.1) -# -# @decompress-threads: Set decompression thread count to be used in -# live migration, the decompression thread count is an integer -# between 1 and 255. Usually, decompression is at least 4 times as -# fast as compression, so set the decompress-threads to the number -# about 1/4 of compress-threads is adequate. -# -# @throttle-trigger-threshold: The ratio of bytes_dirty_period and -# bytes_xfer_period to trigger throttling. It is expressed as -# percentage. The default value is 50. (Since 5.0) -# -# @cpu-throttle-initial: Initial percentage of time guest cpus are -# throttled when migration auto-converge is activated. The -# default value is 20. (Since 2.7) -# -# @cpu-throttle-increment: throttle percentage increase each time -# auto-converge detects that migration is not making progress. -# The default value is 10. (Since 2.7) -# -# @cpu-throttle-tailslow: Make CPU throttling slower at tail stage At -# the tail stage of throttling, the Guest is very sensitive to CPU -# percentage while the @cpu-throttle -increment is excessive -# usually at tail stage. If this parameter is true, we will -# compute the ideal CPU percentage used by the Guest, which may -# exactly make the dirty rate match the dirty rate threshold. -# Then we will choose a smaller throttle increment between the one -# specified by @cpu-throttle-increment and the one generated by -# ideal CPU percentage. Therefore, it is compatible to -# traditional throttling, meanwhile the throttle increment won't -# be excessive at tail stage. The default value is false. (Since -# 5.1) -# -# @tls-creds: ID of the 'tls-creds' object that provides credentials -# for establishing a TLS connection over the migration data -# channel. On the outgoing side of the migration, the credentials -# must be for a 'client' endpoint, while for the incoming side the -# credentials must be for a 'server' endpoint. Setting this will -# enable TLS for all migrations. The default is unset, resulting -# in unsecured migration at the QEMU level. (Since 2.7) -# -# @tls-hostname: hostname of the target host for the migration. This -# is required when using x509 based TLS credentials and the -# migration URI does not already include a hostname. For example -# if using fd: or exec: based migration, the hostname must be -# provided so that the server's x509 certificate identity can be -# validated. (Since 2.7) -# -# @tls-authz: ID of the 'authz' object subclass that provides access -# control checking of the TLS x509 certificate distinguished name. -# This object is only resolved at time of use, so can be deleted -# and recreated on the fly while the migration server is active. -# If missing, it will default to denying access (Since 4.0) -# -# @max-bandwidth: to set maximum speed for migration. maximum speed -# in bytes per second. (Since 2.8) -# -# @avail-switchover-bandwidth: to set the available bandwidth that -# migration can use during switchover phase. NOTE! This does not -# limit the bandwidth during switchover, but only for calculations when -# making decisions to switchover. By default, this value is zero, -# which means QEMU will estimate the bandwidth automatically. This can -# be set when the estimated value is not accurate, while the user is -# able to guarantee such bandwidth is available when switching over. -# When specified correctly, this can make the switchover decision much -# more accurate. (Since 8.2) -# -# @downtime-limit: set maximum tolerated downtime for migration. -# maximum downtime in milliseconds (Since 2.8) -# -# @x-checkpoint-delay: The delay time (in ms) between two COLO -# checkpoints in periodic mode. (Since 2.8) -# -# @block-incremental: Affects how much storage is migrated when the -# block migration capability is enabled. When false, the entire -# storage backing chain is migrated into a flattened image at the -# destination; when true, only the active qcow2 layer is migrated -# and the destination must already have access to the same backing -# chain as was used on the source. (since 2.10) -# -# @multifd-channels: Number of channels used to migrate data in -# parallel. This is the same number that the number of sockets -# used for migration. The default value is 2 (since 4.0) -# -# @xbzrle-cache-size: cache size to be used by XBZRLE migration. It -# needs to be a multiple of the target page size and a power of 2 -# (Since 2.11) -# -# @max-postcopy-bandwidth: Background transfer bandwidth during -# postcopy. Defaults to 0 (unlimited). In bytes per second. -# (Since 3.0) -# -# @max-cpu-throttle: maximum cpu throttle percentage. Defaults to 99. -# (Since 3.1) -# -# @multifd-compression: Which compression method to use. Defaults to -# none. (Since 5.0) -# -# @multifd-zlib-level: Set the compression level to be used in live -# migration, the compression level is an integer between 0 and 9, -# where 0 means no compression, 1 means the best compression -# speed, and 9 means best compression ratio which will consume -# more CPU. Defaults to 1. (Since 5.0) -# -# @multifd-zstd-level: Set the compression level to be used in live -# migration, the compression level is an integer between 0 and 20, -# where 0 means no compression, 1 means the best compression -# speed, and 20 means best compression ratio which will consume -# more CPU. Defaults to 1. (Since 5.0) -# -# @block-bitmap-mapping: Maps block nodes and bitmaps on them to -# aliases for the purpose of dirty bitmap migration. Such aliases -# may for example be the corresponding names on the opposite site. -# The mapping must be one-to-one, but not necessarily complete: On -# the source, unmapped bitmaps and all bitmaps on unmapped nodes -# will be ignored. On the destination, encountering an unmapped -# alias in the incoming migration stream will result in a report, -# and all further bitmap migration data will then be discarded. -# Note that the destination does not know about bitmaps it does -# not receive, so there is no limitation or requirement regarding -# the number of bitmaps received, or how they are named, or on -# which nodes they are placed. By default (when this parameter -# has never been set), bitmap names are mapped to themselves. -# Nodes are mapped to their block device name if there is one, and -# to their node name otherwise. (Since 5.2) -# -# @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty -# limit during live migration. Should be in the range 1 to 1000ms. -# Defaults to 1000ms. (Since 8.1) -# -# @vcpu-dirty-limit: Dirtyrate limit (MB/s) during live migration. -# Defaults to 1. (Since 8.1) -# -# Features: -# -# @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period -# are experimental. +# TODO: elide from generated documentation (type is used only +# internally, and not visible in QMP) # # Since: 2.4 ## { 'enum': 'MigrationParameter', - 'data': ['announce-initial', 'announce-max', - 'announce-rounds', 'announce-step', - 'compress-level', 'compress-threads', 'decompress-threads', - 'compress-wait-thread', 'throttle-trigger-threshold', - 'cpu-throttle-initial', 'cpu-throttle-increment', + 'data': ['announce-initial', + 'announce-max', + 'announce-rounds', + 'announce-step', + 'compress-level', + 'compress-threads', + 'decompress-threads', + 'compress-wait-thread', + 'throttle-trigger-threshold', + 'cpu-throttle-initial', + 'cpu-throttle-increment', 'cpu-throttle-tailslow', - 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth', - 'avail-switchover-bandwidth', 'downtime-limit', - { 'name': 'x-checkpoint-delay', 'features': [ 'unstable' ] }, + 'tls-creds', + 'tls-hostname', + 'tls-authz', + 'max-bandwidth', + 'avail-switchover-bandwidth', + 'downtime-limit', + 'x-checkpoint-delay', 'block-incremental', 'multifd-channels', - 'xbzrle-cache-size', 'max-postcopy-bandwidth', - 'max-cpu-throttle', 'multifd-compression', - 'multifd-zlib-level', 'multifd-zstd-level', + 'xbzrle-cache-size', + 'max-postcopy-bandwidth', + 'max-cpu-throttle', + 'multifd-compression', + 'multifd-zlib-level', + 'multifd-zstd-level', 'block-bitmap-mapping', - { 'name': 'x-vcpu-dirty-limit-period', 'features': ['unstable'] }, + 'x-vcpu-dirty-limit-period', 'vcpu-dirty-limit'] } ##
Peter Xu <peterx@redhat.com> writes: > On Mon, Oct 16, 2023 at 08:29:58AM +0200, Markus Armbruster wrote: >> Better, because even stupider: drop the feature flags. They have no >> effect on internal use, and there is no external use. >> >> ## >> # @MigrationParameter: >> # >> # TODO: elide from generated documentation (type is used only >> # internally, and not visible in QMP) >> # >> # Since: 2.4 >> ## >> { 'enum': 'MigrationParameter', >> 'data': ['announce-initial', 'announce-max', >> 'announce-rounds', 'announce-step', >> 'compress-level', 'compress-threads', 'decompress-threads', >> 'compress-wait-thread', 'throttle-trigger-threshold', >> 'cpu-throttle-initial', 'cpu-throttle-increment', >> 'cpu-throttle-tailslow', >> 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth', >> 'downtime-limit', >> 'x-checkpoint-delay', >> 'block-incremental', >> 'multifd-channels', >> 'xbzrle-cache-size', 'max-postcopy-bandwidth', >> 'max-cpu-throttle', 'multifd-compression', >> 'multifd-zlib-level', 'multifd-zstd-level', >> 'block-bitmap-mapping', >> 'x-vcpu-dirty-limit-period', >> 'vcpu-dirty-limit'] } > > Didn't work either, unfortunately.. Compile is fine, but I still see the > lines generated in qemu-qmp-ref.7. > > MigrationParameter (Enum) > Values > announce-initial > Not documented > > announce-max > Not documented > > announce-rounds > Not documented > > announce-step > Not documented > > compress-level > Not documented > [...] I didn't claim these will *not* be generated :) > Patch attached. > > Thanks, Solutions: 0. Do nothing The QEMU QMP Reference manual documents MigrationParameter, even though it is not actually visible in QMP. The documentation source is a duplicate. 1. Dumb down MigrationParameter's doc comment (your attached patch) The QEMU QMP Reference manual documents MigrationParameter, even though it is not actually visible in QMP. In addition to useless, the documentation is embarrassing: lots of "Not documented". 2. Make MigrationParameter a C enum instead of a QAPI enum (your v3 patch) The QEMU QMP Reference manual doesn't uselessly document MigrationParameter (it still uselessly documents other internal-only things). We replace 20 lines of QAPI schema by 100 lines of C. 3. Improve the QAPI generator to generate docs only for definitions visible externally, and to require doc comments only then The QEMU QMP Reference manual doesn't uselessly document internal-only stuff. This solution doesn't exist, yet.
diff --git a/qapi/migration.json b/qapi/migration.json index 45d69787ae..eeb1878c4f 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -672,185 +672,6 @@ 'bitmaps': [ 'BitmapMigrationBitmapAlias' ] } } -## -# @MigrationParameter: -# -# Migration parameters enumeration -# -# @announce-initial: Initial delay (in milliseconds) before sending -# the first announce (Since 4.0) -# -# @announce-max: Maximum delay (in milliseconds) between packets in -# the announcement (Since 4.0) -# -# @announce-rounds: Number of self-announce packets sent after -# migration (Since 4.0) -# -# @announce-step: Increase in delay (in milliseconds) between -# subsequent packets in the announcement (Since 4.0) -# -# @compress-level: Set the compression level to be used in live -# migration, the compression level is an integer between 0 and 9, -# where 0 means no compression, 1 means the best compression -# speed, and 9 means best compression ratio which will consume -# more CPU. -# -# @compress-threads: Set compression thread count to be used in live -# migration, the compression thread count is an integer between 1 -# and 255. -# -# @compress-wait-thread: Controls behavior when all compression -# threads are currently busy. If true (default), wait for a free -# compression thread to become available; otherwise, send the page -# uncompressed. (Since 3.1) -# -# @decompress-threads: Set decompression thread count to be used in -# live migration, the decompression thread count is an integer -# between 1 and 255. Usually, decompression is at least 4 times as -# fast as compression, so set the decompress-threads to the number -# about 1/4 of compress-threads is adequate. -# -# @throttle-trigger-threshold: The ratio of bytes_dirty_period and -# bytes_xfer_period to trigger throttling. It is expressed as -# percentage. The default value is 50. (Since 5.0) -# -# @cpu-throttle-initial: Initial percentage of time guest cpus are -# throttled when migration auto-converge is activated. The -# default value is 20. (Since 2.7) -# -# @cpu-throttle-increment: throttle percentage increase each time -# auto-converge detects that migration is not making progress. -# The default value is 10. (Since 2.7) -# -# @cpu-throttle-tailslow: Make CPU throttling slower at tail stage At -# the tail stage of throttling, the Guest is very sensitive to CPU -# percentage while the @cpu-throttle -increment is excessive -# usually at tail stage. If this parameter is true, we will -# compute the ideal CPU percentage used by the Guest, which may -# exactly make the dirty rate match the dirty rate threshold. -# Then we will choose a smaller throttle increment between the one -# specified by @cpu-throttle-increment and the one generated by -# ideal CPU percentage. Therefore, it is compatible to -# traditional throttling, meanwhile the throttle increment won't -# be excessive at tail stage. The default value is false. (Since -# 5.1) -# -# @tls-creds: ID of the 'tls-creds' object that provides credentials -# for establishing a TLS connection over the migration data -# channel. On the outgoing side of the migration, the credentials -# must be for a 'client' endpoint, while for the incoming side the -# credentials must be for a 'server' endpoint. Setting this will -# enable TLS for all migrations. The default is unset, resulting -# in unsecured migration at the QEMU level. (Since 2.7) -# -# @tls-hostname: hostname of the target host for the migration. This -# is required when using x509 based TLS credentials and the -# migration URI does not already include a hostname. For example -# if using fd: or exec: based migration, the hostname must be -# provided so that the server's x509 certificate identity can be -# validated. (Since 2.7) -# -# @tls-authz: ID of the 'authz' object subclass that provides access -# control checking of the TLS x509 certificate distinguished name. -# This object is only resolved at time of use, so can be deleted -# and recreated on the fly while the migration server is active. -# If missing, it will default to denying access (Since 4.0) -# -# @max-bandwidth: to set maximum speed for migration. maximum speed -# in bytes per second. (Since 2.8) -# -# @downtime-limit: set maximum tolerated downtime for migration. -# maximum downtime in milliseconds (Since 2.8) -# -# @x-checkpoint-delay: The delay time (in ms) between two COLO -# checkpoints in periodic mode. (Since 2.8) -# -# @block-incremental: Affects how much storage is migrated when the -# block migration capability is enabled. When false, the entire -# storage backing chain is migrated into a flattened image at the -# destination; when true, only the active qcow2 layer is migrated -# and the destination must already have access to the same backing -# chain as was used on the source. (since 2.10) -# -# @multifd-channels: Number of channels used to migrate data in -# parallel. This is the same number that the number of sockets -# used for migration. The default value is 2 (since 4.0) -# -# @xbzrle-cache-size: cache size to be used by XBZRLE migration. It -# needs to be a multiple of the target page size and a power of 2 -# (Since 2.11) -# -# @max-postcopy-bandwidth: Background transfer bandwidth during -# postcopy. Defaults to 0 (unlimited). In bytes per second. -# (Since 3.0) -# -# @max-cpu-throttle: maximum cpu throttle percentage. Defaults to 99. -# (Since 3.1) -# -# @multifd-compression: Which compression method to use. Defaults to -# none. (Since 5.0) -# -# @multifd-zlib-level: Set the compression level to be used in live -# migration, the compression level is an integer between 0 and 9, -# where 0 means no compression, 1 means the best compression -# speed, and 9 means best compression ratio which will consume -# more CPU. Defaults to 1. (Since 5.0) -# -# @multifd-zstd-level: Set the compression level to be used in live -# migration, the compression level is an integer between 0 and 20, -# where 0 means no compression, 1 means the best compression -# speed, and 20 means best compression ratio which will consume -# more CPU. Defaults to 1. (Since 5.0) -# -# @block-bitmap-mapping: Maps block nodes and bitmaps on them to -# aliases for the purpose of dirty bitmap migration. Such aliases -# may for example be the corresponding names on the opposite site. -# The mapping must be one-to-one, but not necessarily complete: On -# the source, unmapped bitmaps and all bitmaps on unmapped nodes -# will be ignored. On the destination, encountering an unmapped -# alias in the incoming migration stream will result in a report, -# and all further bitmap migration data will then be discarded. -# Note that the destination does not know about bitmaps it does -# not receive, so there is no limitation or requirement regarding -# the number of bitmaps received, or how they are named, or on -# which nodes they are placed. By default (when this parameter -# has never been set), bitmap names are mapped to themselves. -# Nodes are mapped to their block device name if there is one, and -# to their node name otherwise. (Since 5.2) -# -# @x-vcpu-dirty-limit-period: Periodic time (in milliseconds) of dirty -# limit during live migration. Should be in the range 1 to 1000ms. -# Defaults to 1000ms. (Since 8.1) -# -# @vcpu-dirty-limit: Dirtyrate limit (MB/s) during live migration. -# Defaults to 1. (Since 8.1) -# -# Features: -# -# @unstable: Members @x-checkpoint-delay and @x-vcpu-dirty-limit-period -# are experimental. -# -# Since: 2.4 -## -{ 'enum': 'MigrationParameter', - 'data': ['announce-initial', 'announce-max', - 'announce-rounds', 'announce-step', - 'compress-level', 'compress-threads', 'decompress-threads', - 'compress-wait-thread', 'throttle-trigger-threshold', - 'cpu-throttle-initial', 'cpu-throttle-increment', - 'cpu-throttle-tailslow', - 'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth', - 'downtime-limit', - { 'name': 'x-checkpoint-delay', 'features': [ 'unstable' ] }, - 'block-incremental', - 'multifd-channels', - 'xbzrle-cache-size', 'max-postcopy-bandwidth', - 'max-cpu-throttle', 'multifd-compression', - 'multifd-zlib-level', 'multifd-zstd-level', - 'block-bitmap-mapping', - { 'name': 'x-vcpu-dirty-limit-period', 'features': ['unstable'] }, - 'vcpu-dirty-limit'] } - ## # @migrate-set-parameters: # 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, +} MigrationParameter; + +extern const char *MigrationParameter_string[MIGRATION_PARAMETER__MAX]; +#define MigrationParameter_str(p) MigrationParameter_string[p] + +/** + * @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); diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c index 88a8ccb475..0a35a87b7e 100644 --- a/migration/migration-hmp-cmds.c +++ b/migration/migration-hmp-cmds.c @@ -31,6 +31,7 @@ #include "ui/qemu-spice.h" #include "sysemu/sysemu.h" #include "migration.h" +#include "migration/options.h" static void migration_global_dump(Monitor *mon) { @@ -505,7 +506,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) Error *err = NULL; int val, ret; - val = qapi_enum_parse(&MigrationParameter_lookup, param, -1, &err); + val = MigrationParameter_from_str(param, &err); if (val < 0) { goto cleanup; } diff --git a/migration/options.c b/migration/options.c index 12e392f68c..c9b90d932d 100644 --- a/migration/options.c +++ b/migration/options.c @@ -84,6 +84,57 @@ #define DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT_PERIOD 1000 /* milliseconds */ #define DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT 1 /* MB/s */ +const char *MigrationParameter_string[MIGRATION_PARAMETER__MAX] = { + [MIGRATION_PARAMETER_ANNOUNCE_INITIAL] = "announce-initial", + [MIGRATION_PARAMETER_ANNOUNCE_MAX] = "announce-max", + [MIGRATION_PARAMETER_ANNOUNCE_ROUNDS] = "announce-rounds", + [MIGRATION_PARAMETER_ANNOUNCE_STEP] = "announce-step", + [MIGRATION_PARAMETER_COMPRESS_LEVEL] = "compress-level", + [MIGRATION_PARAMETER_COMPRESS_THREADS] = "compress-threads", + [MIGRATION_PARAMETER_DECOMPRESS_THREADS] = "decompress-threads", + [MIGRATION_PARAMETER_COMPRESS_WAIT_THREAD] = "compress-wait-thread", + [MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD] = "throttle-trigger-threshold", + [MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL] = "cpu-throttle-initial", + [MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT] = "cpu-throttle-increment", + [MIGRATION_PARAMETER_CPU_THROTTLE_TAILSLOW] = "cpu-throttle-tailslow", + [MIGRATION_PARAMETER_TLS_CREDS] = "tls-creds", + [MIGRATION_PARAMETER_TLS_HOSTNAME] = "tls-hostname", + [MIGRATION_PARAMETER_TLS_AUTHZ] = "tls-authz", + [MIGRATION_PARAMETER_MAX_BANDWIDTH] = "max-bandwidth", + [MIGRATION_PARAMETER_DOWNTIME_LIMIT] = "downtime-limit", + [MIGRATION_PARAMETER_X_CHECKPOINT_DELAY] = "x-checkpoint-delay", + [MIGRATION_PARAMETER_BLOCK_INCREMENTAL] = "block-incremental", + [MIGRATION_PARAMETER_MULTIFD_CHANNELS] = "multifd-channels", + [MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE] = "xbzrle-cache-size", + [MIGRATION_PARAMETER_MAX_POSTCOPY_BANDWIDTH] = "max-postcopy-bandwidth", + [MIGRATION_PARAMETER_MAX_CPU_THROTTLE] = "max-cpu-throttle", + [MIGRATION_PARAMETER_MULTIFD_COMPRESSION] = "multifd-compression", + [MIGRATION_PARAMETER_MULTIFD_ZLIB_LEVEL] = "multifd-zlib-level", + [MIGRATION_PARAMETER_MULTIFD_ZSTD_LEVEL] = "multifd-zstd-level", + [MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING] = "block-bitmap-mapping", + [MIGRATION_PARAMETER_X_VCPU_DIRTY_LIMIT_PERIOD] = "x-vcpu-dirty-limit-period", + [MIGRATION_PARAMETER_VCPU_DIRTY_LIMIT] = "vcpu-dirty-limit", +}; + +int MigrationParameter_from_str(const char *param, Error **errp) +{ + int i; + + if (!param) { + error_setg(errp, "Missing parameter value"); + return -1; + } + + for (i = 0; i < MIGRATION_PARAMETER__MAX; i++) { + if (!strcmp(param, MigrationParameter_string[i])) { + return i; + } + } + + error_setg(errp, "Invalid parameter value: %s", param); + return -1; +} + Property migration_properties[] = { DEFINE_PROP_BOOL("store-global-state", MigrationState, store_global_state, true),