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 <[email protected]> >>> --- >>> 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
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
