8:22 a.m.
On 09/17/2015 11:31 AM, Michal Privoznik wrote:
So, now we have VIR_ENUM_DECL() and VIR_ENUM_IMPL() macros. They
will define a pair of functions for you to convert from and to
certain enum. Unfortunately, their usage is slightly cumbersome:
int tmp;
virMyAwesome ma;
if ((tmp = virMyAwesomeTypeFromString(str)) < 0) {
virReportError();
goto cleanup;
}
ma = tmp;
Ideally, we could avoid using the dummy @tmp variable:
virMyAwesome ma;
if (virMyAwesomeEnumFromString(str, &ma,
"Unable to convert '%s'", str) < 0)
goto cleanup;
So, the first @str is string to convert from. @ma should point to
the variable, where result is stored. Then, the third argument is
the error message to be printed if conversion fails, followed by
all the necessary arguments (message is in printf format).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virutil.c | 31 +++++++++++++++++++++++++++++++
src/util/virutil.h | 32 +++++++++++++++++++++++++++++---
3 files changed, 61 insertions(+), 3 deletions(-)
Your example shows a comparison of "< 0"; however, your patch 3 examples
change a "<= 0" to a "< 0", e.g. (from patch 3):
- if ((val = virTristateSwitchTypeFromString(ioeventfd)) <= 0) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("unknown disk ioeventfd mode '%s'"),
- ioeventfd);
+ if (virTristateSwitchEnumFromString(ioeventfd, &def->ioeventfd,
+ _("unknown disk ioeventfd
mode '%s'"),
+ ioeventfd) < 0)
where essentially that's converted to:
+
+ if ((type = (convertFunc)(str)) < 0) {
+ if (fmt) {
For the tristate in particular, "0" is absent which is used as a marker
of sorts. It's not an error.
Similarly, for "many" a 0 would convert to some VIR_*_NONE and would be
deemed 'illegal'. Of course the "virtType" example in patch 3 shows a
case where a VIRT_*_NONE is not present (there's another recent patch
stream that wanted to change that...)
I think with a tweak to account for a min required value would be
necessary and perhaps for extra credit a non-zero max value.
As an aside, it looks strange to have an argument repeated. I would
think a majority of error messages are going to use the first argument
of the function to the first argument of the error message anyway.
John
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ee7c229..730f2f8 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2343,6 +2343,7 @@ virUSBDeviceSetUsedBy;
# util/virutil.h
virDoubleToStr;
+virEnumFromString;
virFindFCHostCapableVport;
virFindSCSIHostByPCI;
virFormatIntDecimal;
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 3343e0d..6bb9e35 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -444,6 +444,37 @@ const char *virTypeToString(const char *const*types,
return types[type];
}
+int
+virEnumFromString(const char *str,
+ int *result,
+ virEnumConvertFunc convertFunc,
+ const char *enumName,
+ const char *fmt,
+ va_list ap)
+{
+ int ret = -1;
+ int type;
+ char *errMsg = NULL;
+
+ if ((type = (convertFunc)(str)) < 0) {
+ if (fmt) {
+ if (virVasprintf(&errMsg, fmt, ap) >= 0)
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", errMsg);
+ } else {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("Unable to convert '%s' to %s type"),
+ str, enumName);
+ }
+ goto cleanup;
+ }
+
+ *result = type;
+ ret = 0;
+ cleanup:
+ VIR_FREE(errMsg);
+ return ret;
+}
+
/* In case thread-safe locales are available */
#if HAVE_NEWLOCALE
diff --git a/src/util/virutil.h b/src/util/virutil.h
index 648de28..5dd2790 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -27,6 +27,7 @@
# include "internal.h"
# include <unistd.h>
+# include <stdarg.h>
# include <sys/types.h>
# ifndef MIN
@@ -78,6 +79,16 @@ const char *virTypeToString(const char *const*types,
unsigned int ntypes,
int type);
+typedef int (*virEnumConvertFunc) (const char *str);
+
+int virEnumFromString(const char *str,
+ int *result,
+ virEnumConvertFunc convertFunc,
+ const char *enumName,
+ const char *fmt,
+ va_list ap)
+ ATTRIBUTE_FMT_PRINTF(5, 0);
+
# define VIR_ENUM_IMPL(name, lastVal, ...) \
static const char *const name ## TypeList[] = { __VA_ARGS__ }; \
verify(ARRAY_CARDINALITY(name ## TypeList) == lastVal); \
@@ -90,11 +101,26 @@ const char *virTypeToString(const char *const*types,
return virTypeFromString(name ## TypeList, \
ARRAY_CARDINALITY(name ## TypeList), \
type); \
+ } \
+ int name ## EnumFromString(const char *str, int *result, \
+ const char *fmt, ...) \
+ { \
+ int ret; \
+ va_list ap; \
+ va_start(ap, fmt); \
+ ret = virEnumFromString(str, result, \
+ name ## TypeFromString, \
+ #name, fmt, ap); \
+ va_end(ap); \
+ return ret; \
}
-# define VIR_ENUM_DECL(name) \
- const char *name ## TypeToString(int type); \
- int name ## TypeFromString(const char*type);
+# define VIR_ENUM_DECL(name) \
+ const char *name ## TypeToString(int type); \
+ int name ## TypeFromString(const char*type); \
+ int name ## EnumFromString(const char *str, int *result, \
+ const char *fmt, ...) \
+ ATTRIBUTE_FMT_PRINTF(3, 4);
/* No-op workarounds for functionality missing in mingw. */
# ifndef HAVE_GETUID