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

[Eric Blake]

On 07/14/2014 03:09 AM, Michal Privoznik wrote:
> 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;
> }

Yeah, making the API failsafe by separating error return from value
setting is what we've done elsewhere (such as virStrToLong_*).  It's a
bigger, more invasive change, but may be worth it.  And if we do it, we
can then start sticking enum values inside non-public structs.

> 
> 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).

Or even make the error reporting an optional bool parameter.

It's also a huge patch; I wonder if we can come up with an alternative
name (VIR_ENUM_DECL2 expands to name ## FromString, leaving
VIR_ENUM_DECL expanding to name ## TypeFromString - also nice that it's
a shorter name since it's usage will be longer), where we can then focus
on just a few conversions per patch, until all patches are using the new
name and finally delete the old name, rather than redefining
VIR_ENUM_DECL up front and having to do all the refactoring in a single
patch.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140714/abb72b37/attachment-0001.sig>