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

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list