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

[Daniel P. Berrange]

On Thu, Sep 17, 2015 at 05:31:28PM +0200, 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).

I'm not seeing a hugely compelling reason to pass in the error
message string every time we parse an enum from a string. This
is no better than current code, where every caller has a potentially
different error message reported for the same scenario. I'd like
to see

   virMyAwesome ma;
   if (virMyAwesomeEnumFromString(str, &ma) < 0)
       goto cleanup;

We could introduce an explicit VIR_ERR_INVALID_ENUM_VALUE
error code, and a fixed error message to go with it.

> -# 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);

It'd be nice to avoid generating two different FromString methods
at the same time too. I understand you probably did that to avoid
needing to convert all callers at once. How about we have a 
VIR_ENUM_DECL_ERR macro generate the new format only, and just
convert all callers at a time, for any single enum.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|