12:49 p.m.
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
12:49 p.m.
New subject: [libvirt] [PATCH RFC 1/2] util: Add VIR_ENUM_IMPL_LABEL
This allows passing in a string label describing the enum, which can
be used to autogenerate error messages
Signed-off-by: Cole Robinson <crobinso(a)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
4:17 a.m.
New subject: [libvirt] [PATCH RFC 1/2] util: Add VIR_ENUM_IMPL_LABEL
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(a)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
12:49 p.m.
New subject: [libvirt] [PATCH RFC 2/2] conf: Convert virDomainVirtType to VIR_ENUM_IMPL_LABEL
Signed-off-by: Cole Robinson <crobinso(a)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
4:17 a.m.
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
4:24 a.m.
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 :|
2340
days inactive
2341
days old
5 comments
3 participants
participants (3)
-
Cole Robinson -
Daniel P. Berrangé -
Michal Privoznik