[libvirt] [PATCH 2/3] virutil: Introduce virEnumFromString

[John Ferlan]

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 at 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
>