Re: [libvirt] [PATCH 3/3] virSecurityLabelDef: use enum type for @type

On 11.07.2014 18:59, Eric Blake wrote: > On 07/11/2014 03:32 AM, Michal Privoznik wrote: >> There's this trend in libvirt of using enum types wherever possible. >> Now that I'm at virSecurityLabelDef let's rework @type item of the >> structure so we don't have to typecast it elsewhere. >> >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >> --- >> src/conf/domain_conf.c | 6 ++++-- >> src/security/security_dac.c | 2 +- >> src/util/virseclabel.h | 2 +- >> 3 files changed, 6 insertions(+), 4 deletions(-) > > I'm not quite as sure about this one. This solves the issue of how to > detect errors when parsing strings to enum, but required the use of an > intermediate variable which in turn made the patch a net gain in lines > of code. If someone forgets to use the intermediate variable for > parsing, this backfires. On the other hand, parsing string to enum > should be done in just one location, and that's the location touched by > this patch. I'm 50-50 on whether to take this, so I'd like someone else > to chime in with an opinion. I hear you. This patch is just a refactor. It does not add anything useful nor solve any issue. It's okay if dropped. But the more I think about our vir*TypeFromString() the more I feel we should do something about it. How about making it follow our typical function return pattern: int func() { virMyFavourite x; const char *string; if (virMyFavouriteTypeFromString(string, &x) < 0) { virReportError("unknown value: %s", string); goto error; }

That is, we need this diff: diff --git a/src/util/virutil.c b/src/util/virutil.c index 95d1ff9..40075e9 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -407,15 +407,20 @@ virParseVersionString(const char *str, unsigned long *version, int virEnumFromString(const char *const*types, unsigned int ntypes, - const char *type) + const char *type, + int *val)

} \ - int name ## TypeFromString(const char *type) { \ + int name ## TypeFromString(const char *type, name *val) { \ return virEnumFromString(name ## TypeList, \ ARRAY_CARDINALITY(name ## TypeList), \ - type); \ + type, (int *) val); \ } # define VIR_ENUM_DECL(name) \ And then a tons of follow up patches. Or even make virEnumString() report the error (that could save a lot of virReportError() calls).