Re: [libvirt] [PATCH 2/3] virutil: Introduce virEnumFromString
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 :|