[libvirt] [PATCH RFC 0/2] util: Add VIR_ENUM_IMPL_LABEL
This series adds VIR_ENUM_IMPL_LABEL and a demo conversion. VIR_ENUM_IMPL_LABEL allows passing in a string that briefly describes the value the enum represents, which we use to generate error messages for FromString and ToString function failures. This will allow us to drop a lot of code and unique strings that need translating. Patch 2 is an example usage, converting virDomainVirtType enum. It's not a full conversion for this particular enum, just a demo. The enum creation now looks like VIR_ENUM_IMPL_LABEL(virDomainVirt, "domain type", VIR_DOMAIN_VIRT_LAST, ... FromString failure reports this error for value 'zzzz' invalid argument: Unknown 'domain type' value 'zzzz' ToString failure reports this error for unknown type=83 internal error: Unknown 'domain type' internal value '83' Seems like a win to me but I'm interested in other opinions. This could also be a good BiteSizedTask for converting existing enums Cole Robinson (2): util: Add VIR_ENUM_IMPL_LABEL conf: Convert virDomainVirtType to VIR_ENUM_IMPL_LABEL src/conf/domain_conf.c | 10 ++-------- src/util/virutil.c | 20 ++++++++++++++++---- src/util/virutil.h | 15 ++++++++++----- 3 files changed, 28 insertions(+), 17 deletions(-) -- 2.17.1
This allows passing in a string label describing the enum, which can be used to autogenerate error messages Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/util/virutil.c | 20 ++++++++++++++++---- src/util/virutil.h | 15 ++++++++++----- 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index a908422feb..6d23a26a74 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -444,16 +444,22 @@ virParseVersionString(const char *str, unsigned long *version, int virEnumFromString(const char *const*types, unsigned int ntypes, - const char *type) + const char *type, + const char * const label) { size_t i; if (!type) - return -1; + goto error; for (i = 0; i < ntypes; i++) if (STREQ(types[i], type)) return i; + error: + if (label) { + virReportError(VIR_ERR_INVALID_ARG, + _("Unknown '%s' value '%s'"), label, type); + } return -1; } @@ -540,10 +546,16 @@ virFormatIntPretty(unsigned long long val, const char *virEnumToString(const char *const*types, unsigned int ntypes, - int type) + int type, + const char * const label) { - if (type < 0 || type >= ntypes) + if (type < 0 || type >= ntypes) { + if (label) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown '%s' internal value %d"), label, type); + } return NULL; + } return types[type]; } diff --git a/src/util/virutil.h b/src/util/virutil.h index 1ba9635bd9..345c9e053d 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -76,26 +76,31 @@ char *virIndexToDiskName(int idx, const char *prefix); int virEnumFromString(const char *const*types, unsigned int ntypes, - const char *type); + const char *type, + const char *errmsg); const char *virEnumToString(const char *const*types, unsigned int ntypes, - int type); + int type, + const char *errmsg); -# define VIR_ENUM_IMPL(name, lastVal, ...) \ +# define VIR_ENUM_IMPL_LABEL(name, label, lastVal, ...) \ static const char *const name ## TypeList[] = { __VA_ARGS__ }; \ verify(ARRAY_CARDINALITY(name ## TypeList) == lastVal); \ const char *name ## TypeToString(int type) { \ return virEnumToString(name ## TypeList, \ ARRAY_CARDINALITY(name ## TypeList), \ - type); \ + type, label); \ } \ int name ## TypeFromString(const char *type) { \ return virEnumFromString(name ## TypeList, \ ARRAY_CARDINALITY(name ## TypeList), \ - type); \ + type, label); \ } +# define VIR_ENUM_IMPL(name, lastVal, ...) \ + VIR_ENUM_IMPL_LABEL(name, NULL, lastVal, __VA_ARGS__) + # define VIR_ENUM_DECL(name) \ const char *name ## TypeToString(int type); \ int name ## TypeFromString(const char*type); -- 2.17.1
On 07/26/2018 07:49 PM, Cole Robinson wrote:
This allows passing in a string label describing the enum, which can be used to autogenerate error messages
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/util/virutil.c | 20 ++++++++++++++++---- src/util/virutil.h | 15 ++++++++++----- 2 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c index a908422feb..6d23a26a74 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -444,16 +444,22 @@ virParseVersionString(const char *str, unsigned long *version,
int virEnumFromString(const char *const*types, unsigned int ntypes, - const char *type) + const char *type, + const char * const label) { size_t i; if (!type) - return -1; + goto error;
for (i = 0; i < ntypes; i++) if (STREQ(types[i], type)) return i;
+ error: + if (label) {
If we rewrite all of our enums into _LABEL then @label is always going to be non-NULL. But we need this check for now. We are not there yet.
+ virReportError(VIR_ERR_INVALID_ARG, + _("Unknown '%s' value '%s'"), label, type);
@type can be NULL, therefore NULLSTR(type).
+ } return -1; }
Michal
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/conf/domain_conf.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f94a90fbcc..eb40b7f349 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -103,7 +103,7 @@ VIR_ENUM_IMPL(virDomainTaint, VIR_DOMAIN_TAINT_LAST, "custom-dtb", "custom-ga-command"); -VIR_ENUM_IMPL(virDomainVirt, VIR_DOMAIN_VIRT_LAST, +VIR_ENUM_IMPL_LABEL(virDomainVirt, "domain type", VIR_DOMAIN_VIRT_LAST, "none", "qemu", "kqemu", @@ -19141,10 +19141,7 @@ virDomainDefParseCaps(virDomainDefPtr def, goto cleanup; } if ((def->virtType = virDomainVirtTypeFromString(virttype)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("invalid domain type %s"), virttype); goto cleanup; - } if (!ostype) { if (def->os.bootloader) { @@ -27380,11 +27377,8 @@ virDomainDefFormatInternal(virDomainDefPtr def, VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST, -1); - if (!(type = virDomainVirtTypeToString(def->virtType))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected domain type %d"), def->virtType); + if (!(type = virDomainVirtTypeToString(def->virtType))) goto error; - } if (def->id == -1) flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE; -- 2.17.1
On 07/26/2018 07:49 PM, Cole Robinson wrote:
This series adds VIR_ENUM_IMPL_LABEL and a demo conversion.
VIR_ENUM_IMPL_LABEL allows passing in a string that briefly describes the value the enum represents, which we use to generate error messages for FromString and ToString function failures.
This will allow us to drop a lot of code and unique strings that need translating.
Patch 2 is an example usage, converting virDomainVirtType enum. It's not a full conversion for this particular enum, just a demo. The enum creation now looks like
VIR_ENUM_IMPL_LABEL(virDomainVirt, "domain type", VIR_DOMAIN_VIRT_LAST, ...
FromString failure reports this error for value 'zzzz'
invalid argument: Unknown 'domain type' value 'zzzz'
ToString failure reports this error for unknown type=83
internal error: Unknown 'domain type' internal value '83'
Seems like a win to me but I'm interested in other opinions. This could also be a good BiteSizedTask for converting existing enums
Agreed.
Cole Robinson (2): util: Add VIR_ENUM_IMPL_LABEL conf: Convert virDomainVirtType to VIR_ENUM_IMPL_LABEL
src/conf/domain_conf.c | 10 ++-------- src/util/virutil.c | 20 ++++++++++++++++---- src/util/virutil.h | 15 ++++++++++----- 3 files changed, 28 insertions(+), 17 deletions(-)
I like this. You can count on my ACK. But we should probably let others chime in and express their preference. Michal
On Fri, Jul 27, 2018 at 11:17:13AM +0200, Michal Privoznik wrote:
On 07/26/2018 07:49 PM, Cole Robinson wrote:
This series adds VIR_ENUM_IMPL_LABEL and a demo conversion.
VIR_ENUM_IMPL_LABEL allows passing in a string that briefly describes the value the enum represents, which we use to generate error messages for FromString and ToString function failures.
This will allow us to drop a lot of code and unique strings that need translating.
Patch 2 is an example usage, converting virDomainVirtType enum. It's not a full conversion for this particular enum, just a demo. The enum creation now looks like
VIR_ENUM_IMPL_LABEL(virDomainVirt, "domain type", VIR_DOMAIN_VIRT_LAST, ...
FromString failure reports this error for value 'zzzz'
invalid argument: Unknown 'domain type' value 'zzzz'
ToString failure reports this error for unknown type=83
internal error: Unknown 'domain type' internal value '83'
Seems like a win to me but I'm interested in other opinions. This could also be a good BiteSizedTask for converting existing enums
Agreed.
Cole Robinson (2): util: Add VIR_ENUM_IMPL_LABEL conf: Convert virDomainVirtType to VIR_ENUM_IMPL_LABEL
src/conf/domain_conf.c | 10 ++-------- src/util/virutil.c | 20 ++++++++++++++++---- src/util/virutil.h | 15 ++++++++++----- 3 files changed, 28 insertions(+), 17 deletions(-)
I like this. You can count on my ACK. But we should probably let others chime in and express their preference.
I think its good. My only comment is whether we could come up with a way to gradually convert each file, while still finishing up with VIR_ENUM_IMPL as the macro name. A mass rename at the end is one option, but perhaps theres a more clever approach ? 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 :|
participants (3)
-
Cole Robinson -
Daniel P. Berrangé -
Michal Privoznik