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>
> ---
> 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)
{
size_t i;
if (!type)
return -1;
- for (i = 0; i < ntypes; i++)
- if (STREQ(types[i], type))
- return i;
+ for (i = 0; i < ntypes; i++) {
+ if (STREQ(types[i], type)) {
+ if (val)
+ *val = i;
+ return 0;
+ }
+ }
return -1;
}
diff --git a/src/util/virutil.h b/src/util/virutil.h
index 2bb74e2..670b2e1 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -73,7 +73,8 @@ char *virIndexToDiskName(int idx, const char *prefix);
int virEnumFromString(const char *const*types,
unsigned int ntypes,
- const char *type);
+ const char *type,
+ int *val);
const char *virEnumToString(const char *const*types,
unsigned int ntypes,
@@ -87,10 +88,10 @@ const char *virEnumToString(const char *const*types,
ARRAY_CARDINALITY(name ## TypeList), \
type); \
} \
- 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).
Michal