[libvirt] [PATCH 0/3] Rework our enum parsing

Michal Privoznik

17 Sep 2015 17 Sep '15

3:31 p.m.

Only the first two patches are intended for review. The 3rd one is just an example to show how the code base will shrink in size. Michal Privoznik (3): virutil: Rename virEnum{From,To}String to virType{From,To}String virutil: Introduce virEnumFromString conf: Example for the new pattern src/conf/domain_conf.c | 34 ++++++++++--------------------- src/libvirt_private.syms | 3 ++- src/util/virutil.c | 53 ++++++++++++++++++++++++++++++++++++++---------- src/util/virutil.h | 40 +++++++++++++++++++++++++++++------- 4 files changed, 88 insertions(+), 42 deletions(-) -- 2.4.6

Show replies by date

Michal Privoznik

17 Sep 17 Sep

3:31 p.m.

New subject: [libvirt] [PATCH 1/3] virutil: Rename virEnum{From, To}String to virType{From, To}String

While the function names are more or less correct, I will need them in the next commit. So rename these into virType{From,To}String respectively. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 4 ++-- src/util/virutil.c | 22 +++++++++++----------- src/util/virutil.h | 8 ++++---- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5b3da83..ee7c229 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2343,8 +2343,6 @@ virUSBDeviceSetUsedBy; # util/virutil.h virDoubleToStr; -virEnumFromString; -virEnumToString; virFindFCHostCapableVport; virFindSCSIHostByPCI; virFormatIntDecimal; @@ -2401,6 +2399,8 @@ virTristateBoolTypeFromString; virTristateBoolTypeToString; virTristateSwitchTypeFromString; virTristateSwitchTypeToString; +virTypeFromString; +virTypeToString; virUpdateSelfLastChanged; virValidateWWN; diff --git a/src/util/virutil.c b/src/util/virutil.c index cddc78a..3343e0d 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -419,7 +419,7 @@ virParseVersionString(const char *str, unsigned long *version, return 0; } -int virEnumFromString(const char *const*types, +int virTypeFromString(const char *const*types, unsigned int ntypes, const char *type) { @@ -434,6 +434,16 @@ int virEnumFromString(const char *const*types, return -1; } +const char *virTypeToString(const char *const*types, + unsigned int ntypes, + int type) +{ + if (type < 0 || type >= ntypes) + return NULL; + + return types[type]; +} + /* In case thread-safe locales are available */ #if HAVE_NEWLOCALE @@ -526,16 +536,6 @@ virFormatIntDecimal(char *buf, size_t buflen, int val) } -const char *virEnumToString(const char *const*types, - unsigned int ntypes, - int type) -{ - if (type < 0 || type >= ntypes) - return NULL; - - return types[type]; -} - /* Translates a device name of the form (regex) /^[fhv]d[a-z]+[0-9]*$/ * into the corresponding index (e.g. sda => 0, hdz => 25, vdaa => 26) * Note that any trailing string of digits is simply ignored. diff --git a/src/util/virutil.h b/src/util/virutil.h index 53025f7..648de28 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -70,11 +70,11 @@ char *virFormatIntDecimal(char *buf, size_t buflen, int val) int virDiskNameToIndex(const char* str); char *virIndexToDiskName(int idx, const char *prefix); -int virEnumFromString(const char *const*types, +int virTypeFromString(const char *const*types, unsigned int ntypes, const char *type); -const char *virEnumToString(const char *const*types, +const char *virTypeToString(const char *const*types, unsigned int ntypes, int type); @@ -82,12 +82,12 @@ const char *virEnumToString(const char *const*types, static const char *const name ## TypeList[] = { __VA_ARGS__ }; \ verify(ARRAY_CARDINALITY(name ## TypeList) == lastVal); \ const char *name ## TypeToString(int type) { \ - return virEnumToString(name ## TypeList, \ + return virTypeToString(name ## TypeList, \ ARRAY_CARDINALITY(name ## TypeList), \ type); \ } \ int name ## TypeFromString(const char *type) { \ - return virEnumFromString(name ## TypeList, \ + return virTypeFromString(name ## TypeList, \ ARRAY_CARDINALITY(name ## TypeList), \ type); \ } -- 2.4.6

Michal Privoznik

3:31 p.m.

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

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@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virutil.c | 31 +++++++++++++++++++++++++++++++ src/util/virutil.h | 32 +++++++++++++++++++++++++++++--- 3 files changed, 61 insertions(+), 3 deletions(-) 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 -- 2.4.6

John Ferlan

22 Sep 22 Sep

1:22 p.m.

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

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

Daniel P. Berrange

1:33 p.m.

New subject: [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 :|

Michal Privoznik

17 Sep 17 Sep

3:31 p.m.

New subject: [libvirt] [PATCH 3/3] conf: Example for the new pattern

NOT TO BE MERGED UPSTREAM This is just an example of usage of new EnumFromString() API. It's intentionally incomplete. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 34 +++++++++++----------------------- 1 file changed, 11 insertions(+), 23 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6df1618..31b06b7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4904,11 +4904,10 @@ virDomainDeviceInfoParseXML(xmlNodePtr node, type = virXMLPropString(address, "type"); if (type) { - if ((info->type = virDomainDeviceAddressTypeFromString(type)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown address type '%s'"), type); + if (virDomainDeviceAddressEnumFromString(type, &info->type, + _("unknown address type '%s'"), + type) < 0) goto cleanup; - } } else { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("No type specified for device address")); @@ -7430,8 +7429,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } if (ioeventfd) { - int val; - if (def->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("disk ioeventfd mode supported " @@ -7439,13 +7436,10 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } - 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) goto error; - } - def->ioeventfd = val; } if (event_idx) { @@ -7456,14 +7450,10 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } - int idx; - if ((idx = virTristateSwitchTypeFromString(event_idx)) <= 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("unknown disk event_idx mode '%s'"), - event_idx); + if (virTristateSwitchEnumFromString(event_idx, &def->event_idx, + _("unknown disk event_idx mode '%s'"), + event_idx) < 0) goto error; - } - def->event_idx = idx; } if (copy_on_read) { @@ -14754,11 +14744,9 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } - if ((def->virtType = virDomainVirtTypeFromString(tmp)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("invalid domain type %s"), tmp); + if (virDomainVirtEnumFromString(tmp, &def->virtType, + _("invalid domain type %s"), tmp) < 0) goto error; - } VIR_FREE(tmp); def->os.bootloader = virXPathString("string(./bootloader)", ctxt); -- 2.4.6

3630

Age (days ago)

3635

Last active (days ago)

List overview

Download

5 comments

3 participants

participants (3)

Daniel P. Berrange
John Ferlan
Michal Privoznik