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@xxxxxxxxxx>
> ---
> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list