[libvirt] [PATCH 3/3] virSecurityLabelDef: use enum type for @type
Michal Privoznik
mprivozn at redhat.com
Mon Jul 14 09:09:20 UTC 2014
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 at 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
More information about the libvir-list
mailing list